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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Edit Post: Add block management modal #14224
Edit Post: Add block management modal #14224
Changes from 19 commits
86c88a1
8af0287
f83be63
8fc50d9
c61ccd7
9ac28ec
ae02300
40ffefe
c8d5b98
b28cbb0
b681ecb
f8a1874
98e55eb
32e2490
2de9a7e
8671702
fccc796
f092ff6
25b185c
fbf5410
9ce3def
bfc1a38
6171177
c0b9a23
fc978e5
9297f8d
e194aa0
1555f58
2d9b379
6798964
76fb031
ac202ff
a71f799
3d7de09
6e8072a
0d985a2
dca4049
40c7694
22ad54b
0b45148
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the follow-up PR we should cover this new selector with basic unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can get those in place today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unit tests in bfc1a38.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this translation needs to be further processed to replace
%s
with a block name. It must be fixed before we proceed with merge.By the way, VoiceOver announces this component as a toggle button. I think it should be rather a checkbox. I think I mentioned it already :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some related reflections in my comment at #14224 (comment) :
I had used the Color Palette component as precedent for being a similar sort of custom checkbox / toggle button. I'm also happy to update it, should the checkboxes behavior be more appropriate or supported. If we do, I wonder if we shouldn't also consider (separately) whether Color Palette is correct as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the same resource you linked in your comment. My understanding was that the toggle button is better suited for something more standalone, not sure how to phrase it to be honest. The more I read it, the more it seems to be a viable solution in this context as well. We probably should pick the final solution based on how this is going to look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be nice to develop as an enhancement for
valid-sprintf
(the lack ofsprintf
for a string which contains a placeholder value).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the updated design, we're now just rendering a checkbox, which may render the point moot. That said, there's potentially an open question to whether "checked" associated with a block name makes sense. It relies on contextual information of operating within the Manage Blocks Visibility to understand a "checked block" to mean one which is "visible".