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

multiselect support for dropdown default #2344

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Aug 7, 2023

Description

This pull request fixes the defaulting behavior of a plugin select component with multiselect enabled. Previously, even if the previous selection was valid, the current selection would be overwritten. Now, the previous selection is maintained. To reproduce: in imviz, create a second viewer, open plot options, enable multiselect, make a selection for the viewer, create or remove a viewer.

This type of workflow is more evident in #2341, so that should be rebased on top of this.

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)?

@kecnry kecnry added the plugin Label for plugins common to multiple configurations label Aug 7, 2023
@kecnry kecnry modified the milestones: 3.7, 3.6.2 Aug 7, 2023
@kecnry kecnry force-pushed the select-multi-default branch from 17f5705 to 70018a4 Compare August 7, 2023 17:47
@kecnry kecnry force-pushed the select-multi-default branch from 70018a4 to 3adf5d8 Compare August 7, 2023 17:56
@kecnry kecnry marked this pull request as ready for review August 7, 2023 18:14
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Patch coverage: 82.60% and project coverage change: +0.02% 🎉

Comparison is base (eed8420) 90.63% compared to head (fbee0e0) 90.66%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2344      +/-   ##
==========================================
+ Coverage   90.63%   90.66%   +0.02%     
==========================================
  Files         152      152              
  Lines       17407    17439      +32     
==========================================
+ Hits        15777    15811      +34     
+ Misses       1630     1628       -2     
Files Changed Coverage Δ
...configs/default/plugins/export_plot/export_plot.py 59.44% <69.56%> (+5.70%) ⬆️
jdaviz/core/template_mixin.py 91.18% <90.00%> (-0.01%) ⬇️
...configs/cubeviz/plugins/tests/test_export_plots.py 77.77% <100.00%> (+2.02%) ⬆️
...lt/plugins/plot_options/tests/test_plot_options.py 100.00% <100.00%> (ø)
jdaviz/configs/imviz/tests/test_viewers.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

I confirmed that the workflow you described behaves as expected, code looks fine. Approved.

Copy link
Contributor

@javerbukh javerbukh 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!

@kecnry kecnry merged commit 0bbc1f8 into spacetelescope:main Aug 8, 2023
@kecnry kecnry deleted the select-multi-default branch August 8, 2023 20:18
bmorris3 pushed a commit to bmorris3/jdaviz that referenced this pull request Aug 11, 2023
* multiselect support for dropdown default
* show multiselect dropdown if empty
* add test-case for deleting viewer
@kecnry kecnry added the 💤 backport-v3.6.x on-merge: backport to v3.6.x label Aug 25, 2023
@kecnry
Copy link
Member Author

kecnry commented Aug 25, 2023

@meeseeksdev backport to v3.6.x

meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Aug 25, 2023
kecnry added a commit that referenced this pull request Aug 25, 2023
…4-on-v3.6.x

Backport PR #2344 on branch v3.6.x (multiselect support for dropdown default)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Label for plugins common to multiple configurations Ready for final review 💤 backport-v3.6.x on-merge: backport to v3.6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants