When you install Drupal, you should not have to click "Menu" - by default this should be collapsed. This because in probably 99% of the usecases, you are going to want to open that one anyway. And the potential that people don't find out they need to click that item to access the administration tasks is considerable, even if they do find it quickly - it is an unnecessary hump and regression from the Drupal 7 experience.
Comment | File | Size | Author |
---|---|---|---|
#17 | default_state_of-1850164-17.patch | 3.37 KB | Tom Verhaeghe |
#17 | interdiff-15-17.txt | 855 bytes | Tom Verhaeghe |
Comments
Comment #0.0
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #1
Wim LeersBojhan, if you still think we should do this, then we can do this tomorrow at DC Ghent 2014.
One problem with this proposal is that we must NOT do this on narrow viewports, because there the vertical tray would be used. Which means we'd lose a lot of screen real estate. So it'd have to be a "smart default".
Comment #2
Bojhan CreditAttribution: Bojhan commentedYup, lets do this!
Comment #3
Wim LeersCool — clarifying title then :)
Comment #4
Wim LeersWhoever takes this on: #2103247: Clicking menu links in the administration menu tray should close the admin menu tray, while in a narrow viewport where the toolbar is positioned on top of the content is somewhat related, please read that for inspiration on how to do this.
Comment #5
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedComment #6
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI'm not really sure this is a good attempt to fix the problem. With this patch the first menu tab wil be 'opened' by default if no tab was explicitly clicked before and only when the orientation of the tray is horizontal. I also stumbled upon some issues here:
Comment #7
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedFixing inconsistency in comment.
Comment #8
Wim LeersThat's indeed the biggest problem in the patch.
We basically want to always get the first tab that is not the "Home" tab, which is a special snowflake, and if it exists, it's always going to be first. So we need a clean way of detecting it: we want a class to be set on that toolbar tab, just like there already is a class being set on the "shortcut" and "tour" toolbar tabs. Once we have that, we can use this selector and use the first matching element, and the ID for that element is the tab we want to activate:
To add that class, do this:
Comment #9
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedThanks for the pointers, Wim. In this patch I added a class to the 'back to site' tab in the toolbar and now the first element is selected correctly.
Comment #10
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedComment #11
Wim Leersshould mention that we exclude the 'home' toolbar tab.
== 'horizontal'
Please use strict equality.
Comment #12
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedComment #13
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedComment #14
Wim LeersOnly one nitpick remains, then this is RTBC — thanks for your work! :) I manually tested this, and it works perfectly :)
80 cols rule.
Comment #15
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI tried to find it but I guess I'm looking over something. The longest line in the comments I made is 75 columns wide. However, there are some comments in
toolbar.js
(that I didn't write) that violate this rule. Fixed in patch.What line is it exactly in?
Comment #16
Wim LeersWe don't only say that the *maximum* length is 80 cols, we also say that *if* a word on a following line fits on the previous line without exceeding 80 columns, that it should be on the previous line. *That* is what you violated.
Generally, we don't fix unrelated coding standards violations like you did in #15, because it makes reviews too difficult. This patch is small enough to do that though, so it's fine.
If you can just fix #14 also with the extra info I just gave you, this will be RTBC :)
Comment #17
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedFixed #14 (80 columns rule)
Comment #18
Wim LeersAwesome, thank you!
Comment #19
Bojhan CreditAttribution: Bojhan commentedGreat work, thanks @Tom and @Wim!
I am reclassifying this too, because it has a negative impact on the first-time experience (we observed this in the usability test for removing the overlay). As people look were they should go to engage with the system, this is a hurdle less.
Comment #20
alexpottThis issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 232c10d and pushed to 8.0.x. Thanks!