Polish block manager search. #32922
Merged
Polish block manager search. #32922
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
|
@stokesman or @gwwar if any of you have time or inclination, I'd love your help with the second part of this one, extracting a generic search component. I have an idea of the basics of doing so, but my experiene with regards to handling the state of things is still early. If you have time, please feel free to push directly to the branch. |
7 tasks
|
Wonderful!
We don't need to do this now for this pass. |
|
Size Change: +156 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
The code changes here look good, there's a snapshot to update for the unit tests. |
|
Thank you, should be updated! |
|
The design of the search is a bit different than the inserter (Icons on the right) but I think introducing the shared component should fix that if we think the placeholder as a sufficient change now. |
youknowriad
approved these changes
Jun 23, 2021
|
I'm perpetually on deck to push CSS tweaks and polish to anyone who will work with me on such things. Always happy to follow up. |
|
Let's get this in and I'll try to create that component. |
|
@jasmussen I opened #32935 to explore that component. |
|
I like the changes here. The only thing I'd like to note is all the other section titles in the preferences modal have a suggestive phrasing and now this one is the odd one out. If that inconsistency is at all a concern, maybe later we could look at changing the others. I've never liked the suggestive ones. |
|
That's a great point @stokesman. It's not something I feel super strongly about, but we could do some verbiage polish here and there, for example we might be able to change the "Choose how you interact with blocks" to "Block interactions". I'm not sure if it needs further help text than that, frankly, since there's helptext for both toggles. |
|
Yes, I'd prefer titles to be more direct and adding longer descriptions with more context below. |
|
Since this one is approved, and has a pending sub-PR to be merged in, I think it'd be good to land this one as an interim step. Then I'll follow up with a separate PR to rephrase the other headings to be more direct and with help text. |
youknowriad
added
[Type] Enhancement
7 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.
Description
Polish the block manager, per feedback in #32166 (comment). Notably:
Current status:
To Riad's feedback in this comment:
It would be nice to use this PR to do that, and leverage that new component both for this block manager, and the inserter.
How has this been tested?
Checklist:
*.native.jsfiles for terms that need renaming or removal).