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

Image rotation UI/UX updates #23

Merged
merged 11 commits into from
Nov 27, 2023

Conversation

kecnry
Copy link

@kecnry kecnry commented Nov 15, 2023

Description

This pull request:

  • renames "Links Control" to "Orientation" with deprecation (planned follow-up: WCS linking by default)
  • updates viewer data-menu to use radio buttons instead of badge and use preset icons
  • updates preset icon for default orientation
  • updates v-alert styling for less padding and black on yellow text
  • consistent styling for warnings in Orientation plugin

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

* rename plugin from "Links Control" to "Orientation" (with deprecation)
* align by WCS by default
* fix layer letter assignment to ignore wcs-only layers
* text/layout tweaks in plugin
* data menu to use radio-buttons instead of badge, preset icons throughout dropdown and data-menu
* less padding
* font-size consistent with plugin text
* black on yellow for accessibility
@kecnry kecnry requested a review from bmorris3 as a code owner November 15, 2023 20:30
@github-actions github-actions bot added documentation Improvements or additions to documentation imviz embed labels Nov 15, 2023
(icons no longer change so don't need to be tested)
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (wcs-only-layers@c32ad0e). Click here to learn what that means.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                Coverage Diff                 @@
##             wcs-only-layers      #23   +/-   ##
==================================================
  Coverage                   ?   90.22%           
==================================================
  Files                      ?      160           
  Lines                      ?    19853           
  Branches                   ?        0           
==================================================
  Hits                       ?    17912           
  Misses                     ?     1941           
  Partials                   ?        0           

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

@@ -0,0 +1,186 @@
<template>
<j-tray-plugin
description="Rotate the viewer orientation or choose to align images by pixels."
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
description="Rotate the viewer orientation or choose to align images by pixels."
description="Rotate the viewer orientation or choose to align images by WCS."

The phrasing implies WCS is default, and switch to pixel is the functionality. Reverting for this PR (which can be switched again if/when we change to WCS by default in another PR).

Copy link
Author

Choose a reason for hiding this comment

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

Good point, but I think this should be completely rewritten if we're reverting it for now (since you can't rotate when pixel-linked), sigh.

Copy link
Author

Choose a reason for hiding this comment

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

how about "... or choose to switch between aligning images by WCS (Sky) or pixels." so then we don't need to remember to change it later?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure!

notebooks/concepts/imviz-wcs-rotation.ipynb Outdated Show resolved Hide resolved
notebooks/concepts/imviz-wcs-rotation.ipynb Outdated Show resolved Hide resolved
notebooks/concepts/imviz-wcs-rotation.ipynb Outdated Show resolved Hide resolved
notebooks/concepts/imviz-wcs-rotation.ipynb Outdated Show resolved Hide resolved
@@ -170,7 +170,7 @@ def _get_real_xy(self, image, x, y, reverse=False):
# we aren't actually guaranteed to get a SkyCoord out, just for images
# with valid celestial WCS
try:
link_type = self.get_link_type(image.label)
link_type = self.get_link_type(image.label).lower()
Copy link
Owner

Choose a reason for hiding this comment

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

Just a heads up that this snuck in, in case you didn't mean to. But I don't mind.

Copy link
Author

Choose a reason for hiding this comment

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

I just put this everywhere that there could be ambiguity, whether or not it currently fixes anything.

@kecnry kecnry mentioned this pull request Nov 27, 2023
@bmorris3 bmorris3 self-requested a review November 27, 2023 17:49
Copy link
Owner

@bmorris3 bmorris3 left a comment

Choose a reason for hiding this comment

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

This looks good, thank you!

@bmorris3 bmorris3 merged commit 50e2f28 into bmorris3:wcs-only-layers Nov 27, 2023
11 of 12 checks passed
@kecnry kecnry deleted the wcs-only-ui-ux-tweaks branch November 27, 2023 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation embed imviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants