Make nav editor links content-width instead of expanding #34659
Closed
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
vcanales
added
[Type] Bug
vcanales
requested review from
adamziel,
draganescu,
getdave,
kevin940726,
talldan and
tellthemachines
as code owners
|
Size Change: +19 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
vcanales
force-pushed
the
fix/nav-editor-item-size
branch
from
7f3ffc8
to
9c17ade
Compare
|
I think the proposed changes here looks reasonable, but it looks like this may affect the vertical distance between each item in the navigation editor. CleanShot.2021-09-08.at.09.25.16.mp4Is this also an issue in the published page? Should we also add similar behavior to the vertical navigation block variation in other editors?
|
|
Enabling flex might allow the new |
|
I think the increase of vertical distance is acceptable (it also makes it easier to insert elements between items). The only problem I see with this change (and this is something I hadn't thought of until now), is that the inserter doesn't have a consistent width, since it depends on the item above:
I don't think is a stopper, but it's definitely something we could address here #34035 |
getdave
reviewed
Sep 10, 2021
|
I've just rebased this locally against The main problem I'm seeing with the increased spacing is that it only applies to the first level of items: In general I'm thinking it might be good to have consistent spacing at all levels and I prefer the spacing in The second issue I'm seeing is the trailing appender at root level is now centered with the longest item: |
vcanales
force-pushed
the
fix/nav-editor-item-size
branch
2 times, most recently
from
9ed729e
to
866783f
Compare
|
The spacing issue is due to vertical margin collapse not being a feature of flex layouts, so I'm currently looking into either working around this limitation by halving margins where appropriate, or if unable, finding a way to achieve content-width elements without a flex layout. |
|
@vcanales I know @jasmussen changed some styles to flex and fixed some similar spacing issues to the ones you're seeing, so it'd be interesting to see how this looks after a rebase. |
vcanales
force-pushed
the
fix/nav-editor-item-size
branch
from
866783f
to
2853d62
Compare
|
Addressed the different items spacing problems; one of the issues was a
Worth mentioning that the issue where the squiggly line on empty links was solved elsewhere, as can be seen on the comparison above, making this PR only about having items be content-width. With that in mind, does this change perhaps create more problems than it solves? @javierarce If we move forward, this reverts one of the changes made by @jasmussen in #35234, but doesn't seem to make the regression come back; I'd still appreciate 👀 on it!
@talldan It still required the use of |
jasmussen
approved these changes
Oct 11, 2021
There was a problem hiding this comment.
This tests well for me. Trunk and this branch look mostly the same — trunk:
This branch:
The "clicking outside selects the item" is the same in both, I just tinkered more with it in the second recording.
The wavy underline was also fixed separately by a span wrapper. Nevertheless this is a nice little code cleanup. It reduces the height of items a little bit, but looks good to me.
vcanales
force-pushed
the
fix/nav-editor-item-size
branch
from
988e0fb
to
148404a
Compare
|
Let's merge or close this PR. The navigation screen is no longer actively developed. Related #43620. |
|
Closing, since code will probably be removed. |
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.
Description
Fixes #34036
Make sure Navigation Items don't expand their width within the Navigation Editor, when longer items are present.
How has this been tested?
Screenshots
Types of changes
Bug fix.