-
Notifications
You must be signed in to change notification settings - Fork 76
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
Make mosviz visibility fallback more robust #1808
Make mosviz visibility fallback more robust #1808
Conversation
Codecov ReportBase: 88.06% // Head: 88.04% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1808 +/- ##
==========================================
- Coverage 88.06% 88.04% -0.02%
==========================================
Files 95 95
Lines 10204 10197 -7
==========================================
- Hits 8986 8978 -8
- Misses 1218 1219 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Seems to work as intended now. Tried adding data to the viewer, selecting different rows, removing data from viewer, selecting data again, and no problems. Nice!
ba21d1a
to
d7509c7
Compare
Thanks for the reviews! Did a squash and rebase; will merge after the tests pass |
…8-on-v3.1.x Backport PR #1808 on branch v3.1.x (Make mosviz visibility fallback more robust)
Description
Hi again... 😅
So #1798 didn't quite do what @kecnry and I were expecting, so I'm going to reintroduce the original solution I was proposing. After a much thorough second lookthrough, I'm further convinced this is the correct logic we want.
The reason why the missing row elements were visible was because the mosviz visibility fallback for when the object's row is
None
was to return!returnExtraItems
. This flag is intended for the "show/hide all other data" dropdown. Effectively, if it the row information was missing, it would always show the item as visible.Instead, I propose any data without a row should follow the same visibility rules of whether the user manually visualizes the dataset or not. FWIW, this PR brings the mosviz visibility logic in line with the default fallback:
jdaviz/jdaviz/components/viewer_data_select.vue
Lines 173 to 174 in 105de83
As penance, I won't be marking this one as
trivial
😅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.