Skip to content
New issue

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

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

Already on GitHub? # to your account

active state for toolbar buttons #940

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Oct 18, 2021

Description

This pull request adds support for an active state (orange background) to all toolbar buttons. In doing so, it fixes block button instead of circle styling for toolbar buttons (only visible before when hovering, but required for proper shading for new active state), replaces the plugin icon with hamburger (#527), removes the "X" icon when hammer-screwdriver button is active, and replaces the lock icon in mosviz introduced in #918 (see before and after screenshots below).

Before:
Screen Shot 2021-10-15 at 12 16 56 PM

After:
Screen Shot 2021-10-15 at 12 13 38 PM

Fixes #527

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 change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

* orange background color by applying the "active" class
* rename !collapse -> tools_open (and create data_open and layer_viewer_open)
* allow toolbar-items styling (block buttons in toolbars instead of circles) to pass through tooltip wrapping spans
* replace plugin icon with hamburger
* remove X when hammer-screwdriver is active
* replace lock icon in mosviz with link icon (was temporarily lock/unlocked icon to have two states - this is no longer needed with the active state)
@github-actions github-actions bot added the embed Regarding issues with front-end embedding label Oct 18, 2021
@kecnry kecnry added no-changelog-entry-needed changelog bot directive and removed embed Regarding issues with front-end embedding labels Oct 18, 2021
@kecnry kecnry added this to the 2.1 milestone Oct 18, 2021
@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #940 (8f964f7) into main (1ae6e8e) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #940      +/-   ##
==========================================
+ Coverage   69.37%   69.38%   +0.01%     
==========================================
  Files          69       69              
  Lines        4888     4887       -1     
==========================================
  Hits         3391     3391              
+ Misses       1497     1496       -1     
Impacted Files Coverage Δ
jdaviz/app.py 87.41% <ø> (ø)
...igs/specviz/plugins/line_analysis/line_analysis.py 62.29% <0.00%> (+1.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ae6e8e...8f964f7. Read the comment docs.

@PatrickOgle
Copy link
Contributor

PatrickOgle commented Oct 19, 2021

@kecnry This looks good for the most part. I have one comment/question:

Will a user naturally know to click on the hammer/screwdriver a second time make it go away? Guess this is a question @jkotler or for user feedback.

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.

I am a fan of this. It is certainly more intuitive for me. Two comments below.

  1. Is it intentional that the sub-menu (e.g., region selection) does not have the same active state color scheme?

Screenshot 2021-10-19 115511

  1. Will it be confusing that IMPORT + now visually look like it is always "active"?

Screenshot 2021-10-19 115533

@kecnry
Copy link
Member Author

kecnry commented Oct 19, 2021

@pllim

  1. Is it intentional that the sub-menu (e.g., region selection) does not have the same active state color scheme?

Not intentional or unintentional... we can try to do orange there as well (the plotting toolbar is slightly out of our control as its pulled in directly from upstream, but I think it should be feasible to override the styling since they clearly already have an active state, probably through toggle buttons), but then we may need to change the icon colors and/or the inactive background color as well. This is another good question for Jenn.

  1. Will it be confusing that IMPORT + now visually look like it is always "active"?

I agree, and Jenn said she would open another ticket to reconsider the design and emphasis on the import button.

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.

Given answers in #940 (comment) , my comments can be addressed as future PRs.

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 good to me. Nice work.

@pllim pllim merged commit e76e2be into spacetelescope:main Oct 19, 2021
@kecnry kecnry deleted the toolbar-btn-active-state branch October 19, 2021 16:42
@Jenneh
Copy link
Collaborator

Jenneh commented Oct 19, 2021

@PatrickOgle I am hopeful that users will quickly learn to click on the hammer because all menus will follow that same pattern. That said I think it is something to watch out for during usability testing. If it seems like no one is getting it we can reconsider!

I agree with @pllim 's comment about the secondary header also following the same selection pattern. It would be nice if they were also styled the same.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JDAT-1398: Change Plugin icon to Hamburger
4 participants