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

Change default color for plugin lines and scatter marks to increase visibility #2453

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Sep 13, 2023

The color of a line or scatter mark generated by a plugin is currently hard-coded to be jdaviz blue, which is indistinguishable from the second color in the data color cycler (load more than one e.g. spectrum and you'll see a blue line that's nearly identical to the plugin result color). This can lead to confusion between the plugin previews and the data entries.

This PR sets the default color of plugin-generated lines and scatter marks to the jdaviz "accent" color orange ("#C75D2C"), and removes the third color from the jdaviz color cycler, which is a similar orange to the accent color. This way there's no significant color overlap between data entries and plugin marks.

You can inspect the colors in the cycler (both before and after this PR) in a notebook with:

from jdaviz.utils import ColorCycler
from IPython.display import display_html

cc = ColorCycler()

for i, color in enumerate(cc.default_color_palette):
    display_html(f'<div class= "box" style="background-color:{color}">jdaviz color cycler #{i}</div>', raw=True)

On main, this gives:

Screen Shot 2023-09-13 at 12 58 06

Screenshots

Imviz marks are now orange rather than blue, which is weird at first. I'm not sure it's much different in visibility though.

Screen Shot 2023-09-13 at 12 28 28

In Specviz2D, the first extracted spectrum (in blue) is now distinguishable from the spectral extraction preview (orange on this branch, blue on main):

Screen Shot 2023-09-13 at 12 40 28

In LCviz, Binning plugin previews are now distinguishable from the second-loaded light curve entry (orange on this branch, blue on main):

Screen Shot 2023-09-13 at 12 32 45

Change log entry

  • Is a change log needed? If yes, is it added to 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, maintainer
    should 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.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

@bmorris3 bmorris3 added trivial Only needs one approval instead of two no-changelog-entry-needed changelog bot directive labels Sep 13, 2023
@bmorris3 bmorris3 added this to the 3.7 milestone Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage is 83.33% of modified lines.

Files Changed Coverage
jdaviz/core/marks.py 80.00%
jdaviz/utils.py 100.00%

📢 Thoughts on this report? Let us know!.

@bmorris3 bmorris3 marked this pull request as draft September 13, 2023 16:28
@bmorris3 bmorris3 force-pushed the live-preview-markers branch from 16f20d2 to 8b2ee0d Compare September 13, 2023 16:44
@bmorris3 bmorris3 added UI/UX😍 discussion plugin Label for plugins common to multiple configurations and removed trivial Only needs one approval instead of two no-changelog-entry-needed changelog bot directive labels Sep 13, 2023
@bmorris3 bmorris3 changed the title Make plugin scatter mark colors customizable Change default color for plugin lines and scatter marks to increase visibility Sep 13, 2023
@bmorris3 bmorris3 marked this pull request as ready for review September 13, 2023 16:56
Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Code changes look good to me and I think this change is well-warranted to be worth the slightly jarring initial change if you're used to the old blue.

I'd definitely like to hear opinions from at least @camipacifici and @Jenneh before merging though... and although UI-styling changes often skip the change log, I think we'll want to include this in case it catches anyone off guard.

jdaviz/utils.py Outdated Show resolved Hide resolved
@Jenneh
Copy link
Collaborator

Jenneh commented Sep 13, 2023

Big improvement. I checked this for color blindness accessibility, and it looked good. I approve of this change.

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Approving by proxy from Jenn's comment.

@camipacifici
Copy link
Contributor

Checking with @larrybradley @orifox @PatrickOgle

@camipacifici
Copy link
Contributor

Ok from Ori! Lets see if Patrick and Larry have objections in the next day or so, otherwise this is good to go.

@larrybradley
Copy link
Member

I approve.

Copy link
Contributor

@PatrickOgle PatrickOgle left a comment

Choose a reason for hiding this comment

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

Looks great!

@pllim pllim merged commit 70ad90a into spacetelescope:main Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion plugin Label for plugins common to multiple configurations UI/UX😍
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants