Improve code context menu layout position esp visual stability #22102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cla-bot
bot
added
the
cla-signed
mgsloan
force-pushed
the
code-context-menu-layout-stability
branch
5 times, most recently
from
a0d69f2 to
454d24e
Compare
maxdeviant
reviewed
Dec 16, 2024
| @@ -859,8 +891,10 @@ impl CodeActionsMenu { | |||
| }, | |||
| ) | |||
| .elevation_1(cx) | |||
| .p_1() | |||
| .max_h(max_height) | |||
| // x padding is the same as y. | |||
There was a problem hiding this comment.
We could use a single .p here:
Suggested change
| // x padding is the same as y. | |
| .p(Self::y_padding() / 2.) |
There was a problem hiding this comment.
Ah true, thanks!! Ended up replacing this code with use of ListItem + Popover in #22112
mgsloan
force-pushed
the
code-context-menu-layout-stability
branch
from
454d24e to
986890c
Compare
mgsloan
added a commit
that referenced
this pull request
Dec 16, 2024
This is good for code sharing but also sets up #22102 for making assumptions about popover y padding. Release Notes: - N/A
mgsloan
force-pushed
the
code-context-menu-layout-stability
branch
from
0e48dc9 to
2788636
Compare
* Now decides whether the menu is above or below the target position before rendering it. This causes its position to no longer vary depending on the length of completions * When the text area is height constrained (< 12) lines, now chooses the side which has the most space. Before it would always display above if height constrained below. * Misc code cleanups
mgsloan
force-pushed
the
code-context-menu-layout-stability
branch
from
2788636 to
04ec2b6
Compare
mgsloan
added a commit
that referenced
this pull request
Dec 17, 2024
* Presence of the aside no longer affects position of the context menu.
* Prefers to fit to the right, then on same side of line, then other
side of line, within the following preference order:
- Max possible size within text area.
- Max possible size within window.
- Actual size within window. This is the only case that could cause
it to jump around with less stability.
A further enhancement atop this might be to dynamically resize aside height to fit.
Release notes are N/A as they are covered by the notes for #22102.
Closes #8523
Release notes:
* N/A
mgsloan
added a commit
that referenced
this pull request
Dec 18, 2024
* Presence of the aside no longer affects position or size of the context menu. * Prefers to fit to the right, then on same side of line, then other side of line, within the following preference order: - Max possible size within text area. - Max possible size within window. - Actual size within window. This is the only case that could cause it to jump around with less stability. A further enhancement atop this might be to dynamically resize aside height to fit. Release notes are N/A as they are covered by the notes for #22102. Closes #8523 Release Notes: * N/A
helgemahrt
pushed a commit
to helgemahrt/zed
that referenced
this pull request
Dec 18, 2024
This is good for code sharing but also sets up zed-industries#22102 for making assumptions about popover y padding. Release Notes: - N/A
helgemahrt
pushed a commit
to helgemahrt/zed
that referenced
this pull request
Dec 18, 2024
…industries#22102) * Now decides whether the menu is above or below the target position before rendering it. This causes its position to no longer vary depending on the length of completions * When the text area is height constrained (< 12) lines, now chooses the side which has the most space. Before it would always display above if height constrained below. * Misc code cleanups Release Notes: - Improved completions menu layout to be more stable and use available space better.
helgemahrt
pushed a commit
to helgemahrt/zed
that referenced
this pull request
Dec 18, 2024
) * Presence of the aside no longer affects position or size of the context menu. * Prefers to fit to the right, then on same side of line, then other side of line, within the following preference order: - Max possible size within text area. - Max possible size within window. - Actual size within window. This is the only case that could cause it to jump around with less stability. A further enhancement atop this might be to dynamically resize aside height to fit. Release notes are N/A as they are covered by the notes for zed-industries#22102. Closes zed-industries#8523 Release Notes: * N/A
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now decides whether the menu is above or below the target position before rendering it. This causes its position to no longer vary depending on the length of completions
When the text area is height constrained (< 12) lines, now chooses the side which has the most space. Before it would always display above if height constrained below.
Misc code cleanups
Release Notes: