-
Notifications
You must be signed in to change notification settings - Fork 75
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
orientation: expose create_north_up_east_left/right #3308
orientation: expose create_north_up_east_left/right #3308
Conversation
f9a10ae
to
340a75b
Compare
* :meth:`create_north_up_east_left` | ||
* :meth:`create_north_up_east_right` |
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.
should we remove create_
from the method names since these can create AND/OR set?
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.
I might vote for set_
rather than create_
, since set_
tells you the end result?
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.
unless you pass set_as_orientation = False
🤔... but I think I agree that I still prefer that
cd851e4
to
2381f25
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3308 +/- ##
=======================================
Coverage 88.81% 88.81%
=======================================
Files 125 125
Lines 19030 19041 +11
=======================================
+ Hits 16901 16911 +10
- Misses 2129 2130 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Clear, works well for me. |
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.
This looks great, thanks @kecnry!
""" | ||
template_file = __file__, "orientation.vue" | ||
|
||
# defined as traitlet to allow access from UI - leave fixed |
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.
# defined as traitlet to allow access from UI - leave fixed | |
# defined as traitlet to allow access from UI - leave fixed | |
# since this name is used to identify WCS-only layers elsewhere in jdaviz. |
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.
I replaced the comment with: defined as traitlet in addition to global variable above to allow access from UI - leave fixed
to give more context. Maybe we should define this in one place and import into others if it is an assumed fixed string, but we can do that later 🤷♂️
@bmorris3 @camipacifici - I renamed |
You are right, I am ok with that. |
725f5c2
to
d095af5
Compare
d095af5
to
1413e62
Compare
Description
This pull request exposes
create_north_up_east_left(...)
andcreate_north_up_east_right(...)
methods via the public API of the Orientation plugin, along with API hints.This also renames
set_on_create
toset_as_orientation
in these methods, as it sets them whether or not they are created. This does not however, rename the already publicset_on_create
withinadd_orientation(...)
Docs updates: Orientation plugin API docs
API hints:
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.