data-menu: orientation #3271
data-menu: orientation #3271
Conversation
github-actions
bot
added
the
plugin
kecnry
force-pushed
the
dm-orientation
branch
from
84bcfe2
to
6ee90ff
Compare
| valid = self.labels | ||
| if self.is_multiselect: | ||
| if not isinstance(event['new'], list): | ||
| self.selected = [event['new']] | ||
| return | ||
| if not np.all([item in self.labels + [''] for item in event['new']]): | ||
| if not np.all([item in valid + [''] for item in event['new']]): | ||
| self.selected = event['old'] | ||
| raise ValueError(f"not all items in {event['new']} are one of {self.labels}, reverting selection to {event['old']}") # noqa | ||
| raise ValueError(f"not all items in {event['new']} are one of {valid}, reverting selection to {event['old']}") # noqa | ||
| else: | ||
| if event['new'] not in self.labels + ['']: | ||
| if event['new'] not in valid + ['']: | ||
| self.selected = event['old'] | ||
| raise ValueError(f"{event['new']} not one of {self.labels}, reverting selection to {event['old']}") # noqa | ||
| raise ValueError(f"\'{event['new']}\' not one of {valid}, reverting selection to \'{event['old']}\'") # noqa |
There was a problem hiding this comment.
because of the callbacks in the orientation select in the data-menu, there were cases where self.labels was updating between the if-statement and the error message, resulting in very confusing error messages that 'something' not one of ['something']. This fixes that and also adds quotation marks (especially useful for empty strings), which results in the test changes.
kecnry
force-pushed
the
dm-orientation
branch
from
9fdb1af
to
0b92c29
Compare
kecnry
requested review from
rosteen,
javerbukh,
pllim,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
There was a problem hiding this comment.
Maybe I'm too picky, but I think the "Orientation" label over the dropdown takes up a lot of space, and it's redundant when the default label is "Default Orientation."
I referred back to Jenn's design and I see that the "Orientation" label is there, but it's smaller. Comparing the implementation in this PR:
with Jenn's sketch:
Is it easy to make that label smaller or unnecessary?
|
I'd vote that we handle that in the final UI review/tweaks with her... I agree that it feels a little cluttered, but also am hesitant to override default styles that are used everywhere throughout the app. Once we have API hints (#3274), we will also need some of that space to make the eligible and consistent with other labels. |
|
Looks very good to me! Intuitive and functional. I would not reduce the size of "orientation". I fear it will be too small to read. |
|
The new menu design is very intuitive and functions as described. However, I have one question: the menu doesn’t seem to stay fixed on the selected buttons. Was this part of the ticket? Other than that, it looks great. Thank you for your work! 
|
|
@haticekaratay - it should adjust position on scroll to always be attached to the legend - can you try just opening a fresh imviz, opening the menu, and scrolling? Is there some other workflow you did here that breaks it (if so, can you list the steps or show a video)? |
The menu is attached to the button but shifts when you scroll. It should always be positioned fixed to the button or below the toolbar as it's in the main. Screen.Recording.2024-11-13.at.3.43.41.PM.movThis is what I see in main: Screen.Recording.2024-11-13.at.3.55.53.PM.mov |
kecnry
force-pushed
the
dm-orientation
branch
from
b3aff94
to
306ce37
Compare
|
now rebased on top of #3272 |
|
@haticekaratay - I cannot reproduce and don't see anything here that should have affected that 🤔 can you try again now that its rebased? Is anyone else seeing the same? |
I no longer see it after the rebase. Everything is good now! :) Screen.Recording.2024-11-14.at.9.27.46.AM.mov |
Description
This pull request implements orientation selection (when applicable) in the data menu.
Until enabled, the following is necessary for dev testing:
(Note: icons added to dropdown since screen recording)
Screen.Recording.2024-11-07.at.2.41.12.PM.mov
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-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.
triviallabel.