Absorb block manager within blocks preferences #32166
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: -180 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
update/absorb-block-manager
branch
2 times, most recently
from
a7c338d to
89542ae
Compare
stokesman
added
[Type] Enhancement
|
This is looking very good both in terms of code and design/behavior. The only thing that bothered me a bit is the size of the modal in large screens. It seems it's too small (specially in terms of height) |
|
Nice work as always 👌 This is what I see:
In the GIF I stumble a bit on the fact that the active tab is remembered across opening preferences. That's not specific to this branch, so not something to worry about. Just felt slightly weird that it isn't reset on close. Other than that this is superb, except for that thing Riad mentioned — the height does feel a bit small. Mostly when the height grows smaller at a lower breakpoint. That makes sense at mobile and very small breakpoints. But it doesn't seem like it adds a ton of value that the modal grows less tall beyond the 1440 breakpoint. I know that's an existing issue, but it seems like if we can unify everything beyond medium and upwards on a single height, the goldilocks height, it would address Riad's thought! Very cool to see this, thank you again. |
|
Good call about that modal height. I have removed the height adjustment at the huge breakpoint. I'm glad to adjust further if needed. Now the height matches what the base Modal style is for max-height (which was recently updated in #31639). While I was testing I noticed this annoying gap above the sticky headers can still happen: gap-in-block-manager.mp4I found adjusting line-height can fix it but then found Then I found a tiny bit more ambition and changed a couple of other things:
Hopefully that's all good because it seems better to me. I'm happy to change anything. |
jasmussen
approved these changes
Jun 16, 2021
youknowriad
approved these changes
Jun 16, 2021
|
the unit tests failure are valid, it seems. |
stokesman
force-pushed
the
update/absorb-block-manager
branch
from
646a374 to
fe64843
Compare
Snapshot updated, rebased and fixed an e2e test and all is green. |
|
Go go merge it :) |
|
Looking at this in context I think there are a few design improvements we can make.
|
I think a |
7 tasks
|
Started work on the above bits of feedback in #32922. Note that I went with "Visible blocks" in sentence case, as the other headlines and titles use that. |
4 tasks
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.
Extension of #31191. It has a few additional changes addressing preceding comments (#31191 (comment), #31191 (review)) and proposed improvements.
How has this been tested?
Manually
Screenshots
> medium breakpoint
block-manager-in-blocks-preferences.mp4
< medium breakpoint
block-manager-in-blocks-preferences-small.mp4
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).