Block Support: Update color panel default controls #38511
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
aaronrobertshaw
added
[Type] Enhancement
|
This seems good to me. It's always good to start with basics, and then upgrade if need be. Nice! |
Thanks for the sanity check 👍 I'm fixing up the failed e2es on the color panel switch now but will aim to push that and this PR over the line together early next week. Getting the panel switch completed will also achieve the desired order of panels in the inspector sidebar. |
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
92f53cd
to
e5da492
Compare
aaronrobertshaw
requested review from
ellatrix,
nerrad and
ntwb
as code owners
aaronrobertshaw
force-pushed
the
update/color-panel-default-controls
branch
from
b5022ab
to
f999d69
Compare
|
Size Change: +276 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
update/color-support-to-use-tools-panel
branch
from
e5da492
to
dae9fab
Compare
aaronrobertshaw
force-pushed
the
update/color-panel-default-controls
branch
from
f899180
to
ec75631
Compare
andrewserong
reviewed
Feb 8, 2022
There was a problem hiding this comment.
Nice work going through all of these @aaronrobertshaw! I only found one issue that needs to be fixed (a nested object that looks like it might've been an error during copy & paste 😀)
I've added a bunch of comments on blocks where links feature heavily, where I thought it might be more common for folks to want to interact directly with the link color (primarily where we either have a link toggle that's already visible in the sidebar, or where the block mostly renders links). Totally optional to adjust those ones, we can always follow up in separate PRs if need be.
Let me know if you need me to do further testing!
|
Thanks @andrewserong I appreciate the attention to detail and catching the overzealous copy and paste mistake 👍 I was originally trying to only apply the rules / approach stated on the original issue. Given I've already diverged from that I've added the link control as a default to all the blocks you suggested. I agree that will be the better UX. Personally, I still think there could be an argument for no color controls to be defaults, reducing cluter in the sidebar. |
andrewserong
approved these changes
Feb 8, 2022
There was a problem hiding this comment.
Thanks for going through and doing those updates @aaronrobertshaw, much appreciated!
Personally, I still think there could be an argument for no color controls to be defaults, reducing clutter in the sidebar.
Totally. I don't think the decisions here are all that clear-cut, and there's a delicate balance to be struck between what we expose (and what we assume users will need much of the time), and keeping the UI clean and uncluttered so that it's easier to digest.
Personally, I quite like the balance in this PR, and 🤞 it should be easy enough to make changes in follow-ups if we have feedback that folks would like to make tweaks in another direction.
LGTM!
Base automatically changed from
update/color-support-to-use-tools-panel
to
trunk
February 8, 2022 04:55
aaronrobertshaw
force-pushed
the
update/color-panel-default-controls
branch
from
6778f5a
to
3441e2f
Compare
36 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.
Depends on:
Description
When the color panel is switched to use the
ToolsPanel( #34027 ) we'll need to configure which color support controls should be displayed by default. This PR adds default controls configuration to all blocks currently opting into color support following a few rules set out in earlier discussions.In general, the following considerations informed which controls should be default:
This is all flexible, so if something would be better with or without a given control as a default, we can easily make that change, now or in a follow-up.
Default controls by block
Testing Instructions
Types of changes
New feature, enhancement.
Checklist:
*.native.jsfiles for terms that need renaming or removal).