-
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
Attach select menus #1673
Attach select menus #1673
Conversation
Codecov ReportBase: 87.11% // Head: 87.12% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1673 +/- ##
=======================================
Coverage 87.11% 87.12%
=======================================
Files 95 95
Lines 9887 9894 +7
=======================================
+ Hits 8613 8620 +7
Misses 1274 1274
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. |
Now that #1656 is in, can this be rebased and see if the behavior conflicts with the alignment changed there (see #1656 (comment))? |
@duytnguyendtn Looks like this needs a pretty big rebase, I'll hold on testing until you resolve that. (edit: actually looks pretty simple, just the same thing in a lot of files) |
c1f034f
to
86c7f4d
Compare
FWIW I looked into @javerbukh's hover suggestion and I couldn't figure out how to do it, though the internet suggested it should be easy 😅 @kecnry maybe you know how? |
General thoughts on the issue with text extending beyond the width and truncating:
|
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.
Looks good to me, did a quick grep
and couldn't find any that you missed. I think the advantages outweigh the occasional truncation at small browser widths - in lab I actually couldn't find any cases that were truncated, the list names all fit. Users can always expand the panel a bit.
Yikes, there is a conflict now. Please rebase. |
86c7f4d
to
cc31e7e
Compare
Thanks for the heads up @pllim! I think I fixed it |
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.
Ok, I agree - let's get this in and we can always improve any truncation cases as needed later. I'd still be in favor of getting rid of "Common" since that is the most common (hehe) case of overflow that might be confusing, but we can defer that until later as well, if you'd rather.
This is technically a bug fix, so I think it needs a change log. If this has been annoying someone, they would be happy to see it in the change log as fixed. |
As for side effects of "attaching" very long list, maybe we can open a separate issue for that. |
Co-authored-by: P. L. Lim <[email protected]>
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.
LGTM. Thanks!
Description
Historically, we've always had an issue with dropdown menus floating around when the user scrolls the notebook. When working on #1656, I found an
attach
property that effectively docks the dropdown menu to thev-select
:Before:
ImvizExample.-.Jupyter.Notebook.-.Brave.2022-09-22.17-27-37.mp4
After:
ImvizExample.-.Jupyter.Notebook.-.Brave.2022-09-22.17-25-25.mp4
The only consequence that we have to consider here is entries whose names are larger than the plugin bar. Before, the floating menu could accommodate any sized item by overflowing:
When the menu is attached, the menu is restricted to the size of the
v-select
:Ideally, we could combine the two, attach the menu and allow the overflow, but I haven't found how to do that. I've created this PR as a POC, and to encourage discussion on how we can handle this side-effect
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.