Closed klonos closed 1 year ago
Setting the milestone label to the next minor, since the feature that this improves was merged only into 1.x (for 1.25.0).
PR ready for review/testing/feedback: https://github.com/backdrop/backdrop/pull/4421
Quick feedback as a start:
Not sure. The short label is easier to read.
Here the existing shorter labels are much better, in my opinion.
In principle agreed, but the different indentation levels in the screenshot look cluttered.
Thanks for the feedback @olafgrabienski ππΌ ...it gives me things to think about π€
- Not sure. The short label is easier to read.
The reasoning behind that is that in the current UI we have 3 separate help texts for each of the radio options + another help text for the entire radio set. That's confusing and doesn't work well most of the times - I think that we should be using either the individual labels, or the single label for the entire thing. If we are to have both, then we need #1403 (in order to be able to put the "main" help text right below the label of the radios).
- Here the existing shorter labels are much better, in my opinion.
I watched the video of this week's design/UX meeting, and @stpaultim mentioned the same: that the shorter labels looked better/easier. However, if you were new to Backdrop, and didn't know the "internals" of the Smartmenu library, would the words "Default", "Toggle", "Link" make sesne to you? I know they do now for most of us, but that's because we are "chronic" Drupal/Backdrop users.
I personally had this reaction when I tried to put myself in the shoes of a person new to this UI (in combination with the "Collapsible behavior" label):
So unless you have seen these 3 things in action before, the UI doesn't provide any clear idea - you have to select each of the options, then save the block and the layout, then go to the page with the menu to test and see what it looks like.
In principle agreed, but the different indentation levels in the screenshot look cluttered.
That's interesting! This defeats the purpose of introducing the #indentation
Form API property in the first place π€
Thanks for your reply, @klonos!
Re 1, I guess I hadn't fully understood what you meant to "merge". Now I see what you mean, and I agree that merging "Parent item behavior in collapsible/mobile mode" into "Collapsible behavior" makes sense. As I prefer short labels, I'd suggest however to remove either "mobile" or "collapsible" from the label, and maybe also "item", resulting e.g. in:
Parent behavior in mobile mode
Why do I prefer short labels? Because they are easier to memorize and easy in communication ("Choose the 'Toggle' option."). So, also regarding 2, I'm still tending to short labels. (And improve help text as needed.)
Let's see what others think!
Thanks @olafgrabienski ππΌ ...yes, lets' wait for further feedback/suggestions before making any further changes. I'll make sure to bring this up during our weekly design/UX and dev meetings.
Why do I prefer short labels? Because they are easier to memorize and easy in communication ("Choose the 'Toggle' option."). So, also regarding 2, I'm still tending to short labels. (And improve help text as needed.)
I agree with @olafgrabienski on this that I prefer the shorter labels for the same reason.
As for the indentation, the mobile options actually shouldn't be indented or set based on #states
, because the mobile menu still displays in a special mode regardless of whether the hamburger menu is displayed. That just hides the menu on mobile.
Regarding the labels, I appreciate the goal of adding descriptions and not relying on the persons knowledge of what might be specific tech language. Having said that, I think that the short labels are generally recognized and can be learned and once learned, are much easier to use. Whereas the longer descriptions require one to read them closely every time.
The help text for Toggle is the most problematic, because it assumes you understand the word toggle in the definition (or help text). Maybe:
[] Toggle Parent expands or collapses on each tap without any link functionality.
I filed an alternative PR at https://github.com/backdrop/backdrop/pull/4437 that displays the following:
I left the short labels as @olafgrabienski, @stpaultim, and myself prefer, but reworded the descriptions to use different terminology than the labels.
I removed the #indentation
and #states
that tied the menu behavior and accordion options to the Menu Toggle checkbox, because these options still apply on mobile regardless of whether the menu is hidden inside a hamburger menu.
I reviewed the PR from @quicksketch -- good catch on the fact that this applies on mobile even if there is no hamburger menu. Agreed that this also makes indentation under the hamburger checkbox unnecessary.
Looks great to me. One minor whitespace issue, and some unrelated code warnings (do we need to fix those in this PR?)
Looks like no coding standards problems on the changed lines. It's optional to bring all lines into compliance.
Might as well fix that space before merging, but LGTM:
Oh I see. Thanks I have fixed that!
Thanks for getting this new feature clarified before the release @klonos, @quicksketch, @olafgrabienski, and @stpaultim -- merged into 1.25.0
!
@quicksketch @laryn there's a typo in the text we've introduced I believe:
Parent expands or collapses on each tap and does not act a link.
...should there be an "as" in there? As in "does not act as a link."
If we are to change that, if I put my English-not-my-first-language hat on, this would need some further tweaking:
Parent text is a link, +/- element expands or collapses.
Pinging @olafgrabienski @indigoxela for input, and @jenlampton and @stpaultim for thoughts.
Thanks @klonos, that is definitely a typo. I reworded the last option a little to:
Parent text acts as a link and the +/- element expands or collapses.
Both changes are in https://github.com/backdrop/backdrop/pull/4439.
I've reviewed the PR and it's RTBC from me π
I personally think this setting is still very confusing, but the feature is important and I'm happy to see it get into 1.25.0 - user experience improvements can still continue if warranted, and perhaps with more use, better alternatives will become clear. :)
Great, I merged the follow-up PR.
This is a follow-up for the feature that got into v1.25.0 with #2370.
This is a side-by-side of the current UI vs. the what I propose we change:
List of changes (numbered, so that I can get feedback for each one of the from others):
Parent item's behavior in collapsible (mobile) view.
description into theCollapsible behavior
label of the radios. Something likeParent item behavior in collapsible/mobile mode
.#indentation
property to the radio options and the "Accordion-style" checkbox, so that they are nested under the "Display menu toggle button on small screens" checkbox, on which they depend (via#states
that is).This is currently blocked on #6090.