Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Default Plotly map colors #1721

Merged
merged 11 commits into from
Jan 18, 2024

Conversation

bmingles
Copy link
Contributor

@bmingles bmingles commented Jan 10, 2024

  • Added theme variables for Plotly map colors. Wired them up to the default layout
  • Removed unused chart theme variables
  • Fixed scroll issue in styleguide

Testing
Here's a Python snippet that can be used to see the map colors

import deephaven.plot.express as dx
from deephaven import time_table
import random 

def update(fig):
    pass

sourceh = time_table("PT1S").update(formulas=[
    "X = (float) random.uniform(-90, 90)", 
    "Y = (float) random.uniform(-180, 180)", 
    "Z = (float)random.gauss(3, 3)",
    "l1 = i % 20",
    "l2 = i % 30",
    ])

figs = dx.scatter_geo(
    sourceh, 
    lat="X", 
    lon="Y", 
    by="l1", 
    size="Z",
    color_discrete_sequence=["salmon", "lemonchiffon"],
    projection="natural earth",
    unsafe_update_figure=update
)

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (f919a7e) 46.48% compared to head (2c9b2a8) 46.48%.
Report is 3 commits behind head on main.

Files Patch % Lines
...ackages/code-studio/src/styleguide/SamplesMenu.tsx 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1721      +/-   ##
==========================================
- Coverage   46.48%   46.48%   -0.01%     
==========================================
  Files         617      617              
  Lines       37288    37292       +4     
  Branches     9376     9379       +3     
==========================================
+ Hits        17335    17336       +1     
- Misses      19899    19902       +3     
  Partials       54       54              
Flag Coverage Δ
unit 46.48% <40.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bmingles bmingles linked an issue Jan 10, 2024 that may be closed by this pull request
packages/chart/src/ChartTheme.module.scss Outdated Show resolved Hide resolved
packages/chart/src/ChartUtils.ts Outdated Show resolved Hide resolved
packages/chart/src/ChartUtils.ts Show resolved Hide resolved
@bmingles bmingles force-pushed the 101-default-map-colors branch from a7a9a21 to 6658266 Compare January 16, 2024 16:57
@bmingles bmingles requested a review from mattrunyon January 16, 2024 17:38
@mattrunyon
Copy link
Collaborator

@dsmmcken Should we remove the frame? We can control framecolor, framewidth, and showframe

image

Comment on lines +1932 to +1933
geo: {
showcoastlines: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add geo.bgcolor. I'd say just set it to plot_bgcolor instead of making a separate variable for it

@mattrunyon
Copy link
Collaborator

mattrunyon commented Jan 16, 2024

I changed the bgcolor to plot_bgcolor and set framecolor to red just to illustrate. This is what happens when you zoom out. The current changes don't set geo.bgcolor

The frame is drawn around the map or the entire visible area if it extends past the visible map. So maybe we want a slightly different bgcolor to match the panel background and no frame

image

image

@dsmmcken
Copy link
Contributor

In your example, I would expect the area inside the red line to have the slightly lighter bg color, and everything outside to have the content-bg color.

Then hiding the frame is fine.

@bmingles
Copy link
Contributor Author

I set bgcolor to paper_bgcolor instead of plot_bgcolor so that it gets the outer color and keeps the frame boundary as the delineation of the 2 colors

@bmingles bmingles requested a review from mattrunyon January 17, 2024 17:10
@bmingles bmingles merged commit e8b9f12 into deephaven:main Jan 18, 2024
5 checks passed
@bmingles bmingles deleted the 101-default-map-colors branch January 18, 2024 22:55
@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default map colours
3 participants