jdaviz
jdaviz copied to clipboard
Disable cloning viewers in specviz and specviz2d
Description
This pull request disables cloning viewer tabs in specviz and specviz2d to avoid tab management issues since tabs are disabled for these configs.
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 toCHANGES.rstbefore merge. If no, maintainer should add ano-changelog-entry-neededlabel.
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
triviallabel. - [ ] 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?
- [ ] If new remote data is added that uses MAST, is the URI added to the
cache-download.ymlworkflow? - [ ] 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. Bugfix milestone also needs an accompanying backport label.
- [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 88.14%. Comparing base (648148a) to head (3745dc0).
:warning: Report is 1 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #3876 +/- ##
=======================================
Coverage 88.14% 88.14%
=======================================
Files 194 194
Lines 26625 26627 +2
=======================================
+ Hits 23469 23471 +2
Misses 3156 3156
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
This gets into trouble as there is no way in the configs to restore closed viewers. If we enable tab headers, we must use css to hide the close button on at least some viewers to prevent this situation.
Which viewers? I haven't worked with these much but I'm guessing mosviz and maybe rampviz?
Edit: I just reread your response in the ticket. So your idea is to hide the close button on all viewers that don't have the ability to create a new viewer like specviz, until a second viewer is created.
I think it'll probably be much simpler to just disable cloning in configs.... but yes, there is already some css I think for Imviz that prevents closing either the original viewer or the last remaining viewer.
Owee, I'm MrMeeseeks, Look at me.
There seem to be a conflict, please backport manually. Here are approximate instructions:
- Checkout backport branch and update it.
git checkout v4.4.x
git pull
- Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 964f372be83fa2ac15858cc1c803a074d89d7694
- You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3876: Disable cloning viewers in specviz and specviz2d'
- Push to a named branch:
git push YOURFORK v4.4.x:auto-backport-of-pr-3876-on-v4.4.x
- Create a PR against branch v4.4.x, I would have named this PR:
"Backport PR #3876 on branch v4.4.x (Disable cloning viewers in specviz and specviz2d)"
And apply the correct labels and milestones.
Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!
Remember to remove the Still Needs Manual Backport label once the PR gets merged.
If these instructions are inaccurate, feel free to suggest an improvement.