Improve button block appender. #28464
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
jasmussen
added
[Type] Bug
|
Note that the very widely spaced out buttons blocks is a pre-existing issue. |
|
Size Change: -81 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
ntsekouras
reviewed
Jan 26, 2021
There was a problem hiding this comment.
Thanks Joen!
Things seem to work well here. The only thing that I noticed and I'm not sure we want is this change of position in verticalbuttons:
Before:
After:
Note that the very widely spaced out buttons blocks is a pre-existing issue.
This is something we need to solve as well. Do you think you can come up with an easy fix?
|
Great thoughts, I pushed some fixes:
The issue with buttons is that it's a flex container. There should be a fix, because I believe flex is the right thing to use. But I'll need to look at that separately. |
mkaz
approved these changes
Jan 26, 2021
|
When I add a vertical Navigation block, I see the following strangeness when focusing on the + button:
|
|
Excellent catch, Shaun, of course I should've seen that one coming. Fixing now. |
jasmussen
force-pushed
the
improve/appender-focus
branch
2 times, most recently
from
566b86f to
2f940e7
Compare
|
Alright, should be addressed:
|
jasmussen
force-pushed
the
improve/appender-focus
branch
from
2f940e7 to
6e71665
Compare
aristath
approved these changes
Feb 1, 2021
shaunandrews
approved these changes
Feb 1, 2021
|
Thank you for the reviews. I've rebased and discovered a couple of small things that I need to address. |
jasmussen
force-pushed
the
improve/appender-focus
branch
from
6e71665 to
f8c2950
Compare
|
Alright, after the rebase I noticed a few minor issues in testing:
All of these are fixed now, and it should be good:
Here's some demo content if you'd like to test again: Because of the new changes, I'm going to let this one sit for a day or so, and be open to additional testing if deemed necessary. Otherwise I'll merge it tomorrow. Thanks again! |
6 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.
This is a followup to a previous PR that improved the focus style of the button block appender.
The button block appender is the appender used in horizontal button blocks, such as Social Links, Navigation, Buttons.
This PR does three things:
Before:
After:
When you test this PR, please test every appender. Things should be scoped to touching just the button block appender, but test wider than that regardless.
Also focus specifically on Buttons, Navigation and Social Links blocks. Test both setup states, and fully set up.