Try: Allow vertical inserter in the nav block. #28833
Merged
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
|
Size Change: +69 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
6 tasks
aristath
requested changes
Feb 9, 2021
|
Ems! Sure! |
|
I actually discovered a small issue with the CSS I removed, so I'm going to rebase, and fix that up, and then ask for a last sanity check. But thanks so much for the review so far Ari!
|
jasmussen
force-pushed
the
fix/vertical-inserter-nav
branch
2 times, most recently
from
b2acfa7 to
9c1946f
Compare
|
Alright, should be good:
Note that those margins are spread across three files, and ideally they should be only in style.scss. I will follow up with a separate PR to merge those, so we can both simplify the markup, and have only a single file to override if this is to be customized. |
The styles in |
|
That was my thought exactly. |
6 tasks
|
Created #28878 as an immediate followup. |
|
I'm not sure why the tests keep failing. I'm getting this: not sure why this PR would cause that. |
|
@jasmussen just commit them here (or separately to master) as you wish :) |
|
The margin changes are potentially a bit sensitive, so I figured I'd keep things separate |
@jasmussen I don't think it's this PR 🙏 . It looks like the recent commits into master have been failing for this reason: |
jasmussen
force-pushed
the
fix/vertical-inserter-nav
branch
from
00e4b58 to
f8d9f3f
Compare
|
This makes sense for the horizontal variation, but for the vertical variation (which is also used on the new Navigation screen) this PR adds some unneeded margin:
|
|
Great catch, I'll address that. Thank you Shaun. |
jasmussen
force-pushed
the
fix/vertical-inserter-nav
branch
from
f8d9f3f to
41000a9
Compare
|
Alright, in 2eae54d I pushed some tweaks to margins. But I also discovered that the rabbit hole goes a bit deeper now that the page list has landed. It doesn't make sense to introduce the margin only for the separate menu items. So I'm pausing this one for a bit to give it a bit more time in the oven. In #28265 @tellthemachines wrote:
It sounds like that might simplify some of the CSS classes, so I have to write less edgecases (adding complexity). Once that's further along, it's probably a good time to rebase this one and pick it up again. |
|
Waiting for #29465. |
jasmussen
force-pushed
the
fix/vertical-inserter-nav
branch
3 times, most recently
from
a1860b8 to
0a4f0c8
Compare
|
Alright, this one has been unblocked by the recent refactor of the navigation block. I've rebased, squashed, and simplified, so the changeset is smaller now. The testing instructions are the same — you should see the vertical inserter between menu items in the editor. And you should see a .5em margin between items front and backend:
|
|
But because it changed a bit, I would appreciate a re-review or sanity check! |
draganescu
approved these changes
Mar 12, 2021
jasmussen
force-pushed
the
fix/vertical-inserter-nav
branch
from
0a4f0c8 to
4cd0ad1
Compare
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.
Fixes #28313.
This PR refactors the margins of navigation menu items to have 8px space between them. This allows the vertical sibling inserter to work:
This is a bit of an opinionated change, because:
Penny for your thoughts?
Checklist: