Iterations on options modal #25837
Iterations on options modal #25837
Conversation
karmatosed
added
Needs Design Feedback
|
Size Change: -3.45 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
|
@karmatosed Thanks for working on this! The code implementation looks good to me 👍 Adding the Zoom into the UI a bit... I feel like maybe the margins could be tweaked just a tiny bit? I have a screenshot of the helpText margins tweaked:
The final style was What do you think :) |
|
Great idea, I iterated a little and pushed to PR. Thanks for taking a look @ItsJonQ. Here is where I came to:
|
|
@karmatosed Great :) Peeking at the CSS variables, my initial suggestion was for something like this: That aside... The tests seem a bit unhappy 🙈 . Lemme know if you some help with those 🙏 |
|
Nicely done. Looking at this one:
The spacing below the help text looks more correct in the previous screenshot shared, not sure which one is the latest:
Also should the helptext be not italic, like in the dropdown?
Nice bit of constency to be found in unifying those two. Cool! |
|
Thanks @ItsJonQ and @jasmussen - you are both really helping this take shape! @jasmussen you rightly pointed out after this PR there's a win to be had here in another one for a fix on help text to unify, there's an italic being picked up that for now I've set an override on, but can fix upstream in another PR (I'll get that begun outside this). Here is the current state of PR:
@ItsJonQ that calculation works nicely, thank you for that refinement. I would love help sorting the tests, I'm not totally convinced all are valid either which is perplexing, so would love to work through those. Thank you for offering to help there. |
|
Linking in #25851 as that has the root fix for the font styling so once in can remove the work around in this PR. |
|
Taking a look at the E2E test failures... I'm really not sure why they're breaking 🤔 |
ItsJonQ
force-pushed
the
try/options-iterations
branch
from
536040a
to
f624d5e
Compare
|
@karmatosed I rebased it with latest |
|
Took this for a spin:
I really like this direction and the changes you've made to the margins and style of the helper text 👍 I have some suggestions for some of the helper texts. Please feel free to ignore if these don't make sense to you: For "Pre-publish checklist", how about "Allows review of document settings before publishing."? For "Contain the text cursor", how about "Keeps keyboard cursor inside active block."? For 'Compact interface', how about "Reduces options in toolbar and outlines."? |
karmatosed
requested review from
ajitbohra,
gziolo,
nerrad and
ntwb
as code owners
There was a problem hiding this comment.
Hey @karmatosed - thanks! I pushed a couple of commits to fix the failing tests.
|
Thanks so much @ntsekouras for your help there. @enriquesanchez those look like some great suggestions, let me work on an iteration. Thank you. |
|
One other potential iteration I have been working on is to change up the sections. @enriquesanchez I would love your input here if that is possible as I know you are working on a keyboard shortcut section, that to me feels like this option doesn't fit there (although it might). In the explorations for the iterated options panel, the sections included 'blocks' and this could be something explored here:
|
|
Not bad. Not for this PR, but what would it look like without those borders, perhaps slightly more space instead? |
|
I agree that a whole section just for the option of 'Contain the text cursor' feels like too much, but I also don't find that it fits with any of the others. Have we considered having an 'Accessibility' section in here? I think both 'Contain the text cursor' and 'Display button text labels' could fit well under it. |
|
@jasmussen I think you have a great point there and maybe a middle ground is to remove the borders for now in the content area, leave the header? I have pushed a version of that to the PR just to see, happy for feedback around that as it's something we can always reverse out if want to. Here is what I ended up with adding a small amount of spacing increase:
|
|
I like the reduced borders there! Seems good to me 👍 👍 |
karmatosed
requested review from
chrisvanpatten and
youknowriad
as code owners
There was a problem hiding this comment.
🚀 from me! Thanks for helping with this @karmatosed + @jasmussen
karmatosed
force-pushed
the
try/options-iterations
branch
from
eaa2b01
to
8427221
Compare
|
Noting that above is rebasing so it's in sync. Thanks for reminder @ZebulanStanphill. |
|
Just looking at the images presented here, I don't understand what "Most used blocks in library" is |
|
@carolinan Yeah, the wording seems a bit unclear to me as well. I think "Show Most Used section in block library" or something like that would be clearer. |
|
@carolinan thanks for the suggestion. The idea was the checkbox would be the activator there. I am very happy to add back in 'show' but do you feel the same for the button text label also? I think both of those being consistent makes sense. Mostly, I think avoiding a sea of 'shows' is key here so let's explore further. |
|
Much cleaner look without the borders. The copy is still rather hard to parse tho. I would take another pass to make the labels more descriptive, and where its needed, add helper text. I also think you could consider adding helper text to some or all of the section headers. (For example, I don't think everyone knows what or where the "Document panel" is.) I'd be careful that the helper text is not just rephrasing the label. "Contain the text cursor" is one example of this where the helper text is not necessarily clarifying. Maybe the helper text could instead say why you'd show/hide this. |
|
I took the feedback and iterated, here you can see the results:
Things I changed:
Huge thanks to @enriquesanchez for working with me on the copy and to @ItsJonQ for guidance on adding to the component descriptive text, running tests. @kellychoffman I would love to get your opinion on this as feel it's in a place to at least get in for release. Would love your thoughts there. @ItsJonQ would love a code review once the tests have finished if that is possible. |
|
I think "preventing text caret leaving blocks" reads weirdly... I think "preventing text caret from leaving blocks" would be better. We should probably note that "Use theme styles" may have no effect if the theme doesn't support the feature. The term "post panel" isn't used anywhere else, and may not make as much sense on other post types. Other than those three things, I like this. It's certainly an improvement over |
|
👍 for the new |
|
@ZebulanStanphill Thanks for your input. Regarding:
I wonder if we should then not even show this if the theme doesn't support it? It seems unexpected to me to have a feature that you might not be able to use? I wonder if there already is a check though? |
|
I just did one more iteration based on last round of feedback. You can see the changes here:
|
|
|
|
As the tests seem to have now passed after @youknowriad's help, thanks to everyone for all the feedback and work in getting here. I am going to merge this! Excited to get this iteration in. |
Perhaps we could put |
|
Nice to see this iteration! I think we need to look at better grouping next — for example, button labels in "General" doesn't make much sense. It could be better in "Appearance". Also the "Keyboard" grouping seems awkward to me. Also "theme styles" should be at the bottom in that group. |
|
@mtias no problem I can spin up an iterative PR to bring that in now this is in. I'll do that now. |
This responds to feedback on #25837 and iterates the grouping.
This responds to feedback on #25837 and iterates the grouping.
A few iterations to the options modal in line with #24965 as a step for this release, whilst the new design is being worked on.
This PR is starting to focus on the points highlighted in the comment by @mtias:
Changes
I have done some copy changes and worked on bringing in the same style for help text, however, for now, kept the checkboxes. One point to note that could be done if copy is agreed in time is moving also to toggles for this release.
Here you can see a visual of what this PR results in:
Feedback
I don't consider this to be a final PR, I would love feedback and here are a few areas to focus that, although general feedback is as always welcome.
I would also love a code review as there is some style adding and I need to be sure doing that correctly in this case. Work will continue with the design over on the issue, but let's get something in for this release as an iterative step. Thanks in advance.
Next steps
Once I have feedback, I want to work to within a few days getting something merged for this as we can always iterate from this point.
Thanks
Props to @jasmussen and @ItsJonQ for helping me working through some code hunting and setup fun to get this PR brewed.