-
Notifications
You must be signed in to change notification settings - Fork 79
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
Docs data menu #3427
base: main
Are you sure you want to change the base?
Docs data menu #3427
Conversation
Open questions:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3427 +/- ##
=======================================
Coverage 87.53% 87.53%
=======================================
Files 128 128
Lines 19957 19962 +5
=======================================
+ Hits 17469 17474 +5
Misses 2488 2488 ☔ View full report in Codecov by Sentry. |
I do like the orange it does play nicely into the color them of the app and I think there is enough contrast with light mode browser that it shouldn't be an issue reading. The color theme for labels as such are already discontinuous in the documentation so I don't see an issue updating to orange as it is a continuous them for the different config toolbars.
The Display Settings might be tricky to fit in, but I do like the idea of adding it to both. I think it is comprehensible as is but would solidify to the users that the flux and uncertainty viewers have the same tools without having to make the connection themselves. Everything else looks good in the PR, so I'll wait to hear back and test once more locally before approving if there is an update made to any of the images. |
Better? Or too crowded now? Happy to iterate again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the updates look good, and I like the location of the for "Data Selector" with the old Data Menu!
Thank you for the review! |
Updated screenshots with the new data menu.
Fixes #
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.