Problem/Motivation
We punted on this for the initial commit, because it gets into a lot of questions regarding how to properly do this so it interacts well with lots of different themes.
However, currently, the off-canvas tray looks like this:
And it needs to look more like this:
Proposed resolution
Update both module and theme CSS to accurately reflect the designs/
Remaining tasks
Size and position (module)
- Make summaries and details elements span the full width of the tray
- Remove margin from summaries and details so that they collapse together when closed
- Increase the line height of summaries so that they are more readable
- Remove unneeded padding top and bottom
- reduce the size of descriptions and make darker
Font and color (theme)
- Make font match toolbar (lucida stack from Seven theme)
- Make tray background dark (#444)
- Make type light (#ddd to match toolbar)
- Make link color lighter
- Add pencil icon to tray title (#fff)
- Change color of close button (#ddd)
- Remove noisy styling from table (no cell backgrounds)
- Collapse cell borders
- Make table full width
- Add back table header
- Make th font normal
- Add back table row lines (above 1px solid #555)
- Style buttons (match quickedit)
- Style primary button (match quickedit)
- Override standard dropbutton styling (no gradients)
Out of scope for this issue
- Change table-drag icon
- Load block config in separate container
- Style config container header with wrench icon
User interface changes
The tray gets a dark theme and tray elements are "morticed" up to the edges of the tray to provide more horizontal space.
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#58 | interdiff-2781577-outsidein-55-58.txt | 628 bytes | tkoleary |
#58 | 2781577-outsidein-58.patch | 36.87 KB | tkoleary |
#56 | 2781577-outsidein-55.patch | 36.7 KB | tkoleary |
#48 | Screen Shot 2016-09-19 at 7.08.41 AM.png | 144.23 KB | tkoleary |
#44 | interdiff-2781577-37-43.txt | 31.48 KB | tkoleary |
Comments
Comment #2
xjmPosted another potential solution (or interim improvement): #2782345: Use the admin theme for the Outside In sidebar
Comment #3
tkoleary CreditAttribution: tkoleary at Acquia commentedI think perhaps we should have two issues for this because there are really two somewhat different problem sets.
One is that form elements in the off canvas tray need some CSS that makes them "fit" the given space without overflow and in a more efficient and elegant arrangement.
The other is that we do not want the form in the tray to simply inherit the styling of the theme both to visually disambiguate the creation (site) from the tool (configuration form) for the user, and to avoid unintended gotchas because a front-end them could do anything, including .field-item {display: none;}.
These, it seems to me, should be handled in that order.
Comment #4
webchickComment #5
tedbowComment #6
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #7
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #8
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #9
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #10
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #11
Bojhan CreditAttribution: Bojhan as a volunteer and commentedThis is looking great, I hope we can get it in. Kevin could you share source and full screenshot (with the full page).
I am just wondering a few things:
1) In the sidebar we use a white background, here a dark grey. I prefer dark grey, but should the two visually match, or not?
2) I wouldn't do custom checkboxes - they are a hell to maintain, if we do it - we should do it consistently across core.
3) There is no separation between the button and the "menu levels", where there is between others - I would prefer if we are consistent and always have a line between larger elements.
4) There are like 4 different styles around the button "menu levels", "advanced", "submit button", and "tools block" this looks a bit messy - can we have a consistent accordion style and place it all in there, and maybe one diverting style for the "advanced" which takes people elsewhere?
5) Why use new drag and drop slider? That's scope creep.
It looks like quite some few elements here wouldn't pass WCAG 2.0.
Would love to help out, perhaps we can schedule a meeting to just do some design brainstorming together.
Comment #12
tkoleary CreditAttribution: tkoleary at Acquia commentedI don't think they need to match necessarily. The thinking behind the dark background is to differentiate it from the site. Of course there are sites with dark backgrounds but they are a minority. At a later point we might add a configuration option for a light background and my specs file contains styles for that should we need them. This is something that may apps do.
Those are Chrome's standard checkboxes. They just have box-shadow added (can be done with CSS) to pop them off the background.
There is a background color change but it's subtle. I'm ok with adding a line.
Good idea.
For table drag that icon is becoming much more common than the one we use, which tends to be used for omni-directional drag and drop. What's more important to me is that there is still no good affordance for indent which I'd like to address with an additional icon, but I think that should also be a different issue.
I'm ok with making the table drag icon a different issue as well.
@tim.plunkett already pointed that out and I went through and made adjustments and re-tested. It all passes now.
Sure, just let me know some times that would work for you.
Comment #13
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
Uploaded an MVP spec as pdf
I can also send a sketch file if you like.
Comment #14
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #15
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #16
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #17
tim.plunkettHacks are needed for now, since \Drupal\Core\Asset\AssetResolver::sort() and \Drupal\Core\Asset\LibraryDiscoveryParser::buildByExtension() contradict each other.
When sorting by group, AssetResolver::sort() says
But when actually parsing the library to set the group, LibraryDiscoveryParser::buildByExtension() says:
And then it overwrites the 'group':
I've added an @todo, but it needs a follow-up issue.
Also needed a workaround for #2793343: Dialog drupalAutoButtons option should be respected on initial load.
The major parts left undone are:
For 4/5, not sure why one is a pencil and one is a wrench, also why does the text differ?
Also, is the submit button really supposed to say "Save site details" for all of these? More guidance is needed there.
Finally, the comps completely ignore the Title field and the Display Title checkbox. So I left them in.
Comment #19
Wim LeersAs one of the de facto maintainers of the asset library system, confirmed @tim.plunkett's analysis in #17. See #1945262-51: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering for my analysis, plus the solution. Until #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering lands, this is the best possible solution.
Comment #20
tim.plunkettWith some help from Wim, I confirmed that the hook_css_alter() is the best approach right now.
#1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering is the issue to fix that.
Also fixed the tests to adjust for the workaround for #2793343: Dialog drupalAutoButtons option should be respected on initial load.
Comment #21
tim.plunkettChanged the title of the tray to be "Configure @block", and removed it from the bottom.
Changed the submit button to "Save @block".
Added the pencil icon to the titlebar.
Fixed some CSS issues when used with a theme that doesn't extend Classy.
Comment #22
tim.plunkettReviewed the color contrast, everything passes at least WCAG AA now.
Also fixed an issue with the weights on the branding block.
As far as I'm concerned, this is done.
Comment #23
tkoleary CreditAttribution: tkoleary at Acquia commentedSome detailed notes:
Comment #24
webchickSounds like a "needs work."
Comment #25
tim.plunkettThe show row weights link is already that color.
I'm not changing the input:focus, let's discuss that in a follow-up. It is very disorienting, and non-standard.
We can't style .details-wrapper, since it's non-standard markup that differs between subthemes of Stable and subthemes of Classy.
Also skipping "Style select box for dark background" as that isn't cross-browser safe and is also disorienting. We shouldn't be changing the look-and-feel of standard input elements.
Changed everything else.
Comment #26
tkoleary CreditAttribution: tkoleary at Acquia commentedThat's lifted directly out of Seven theme.
We also already do re-style the look and feel of standard input elements in Seven theme. These arguments have already come up and been resolved.
Comment #27
tim.plunkettCopied that bit from Seven, with the scoping.
There is no
input[type="select"]
anywhere in core, nor is the color #777 used in Seven or anywhere else.Comment #28
tkoleary CreditAttribution: tkoleary at Acquia commentedThis is the re-styling of selects from seven theme.
It's in components/form.css
If I recall correctly it's scoped to webkit because styling selects was only supported in webkit at that point.
Essentially what we should be going for is a "dark" version of this.
Comment #29
Bojhan CreditAttribution: Bojhan as a volunteer and commented@kevin Just wondering - the "focus of eye" should be on the Edit Menu Tools part in this interaction (at least thats what I am assuming is the 80%).
Given this. I imagine we should flip the greys. Making the core interaction light grey, and the less important parts use dark grey.
I am not totally a fan of all the different border colors, not sure if we need them to pass WCAG AA.
Either way this is already 100% better than the current, so love to see it go in!
Comment #30
tkoleary CreditAttribution: tkoleary at Acquia commented@TimPlunkett
I'm ok moving the select stuff to a follow-up issue, though, and calling this one complete. It checks all the boxes of the MVP design.
Comment #31
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
I tested for WCAG at the design stage and I believe everything is passing at at least AA if not AAA.
Comment #32
Bojhan CreditAttribution: Bojhan as a volunteer and commentedCould you also read the other parts of my comment? :P
Comment #33
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
I agree that the focus of the eye should be where the primary action is, but to my eye that *is* the dark area (inside the details wrapper), which has more contrast than the lighter areas.
Comment #34
riddhi.addweb CreditAttribution: riddhi.addweb commentedComment #35
tkoleary CreditAttribution: tkoleary at Acquia commented@Bojhan
Opened a follow-up issue for validating the visual design of the tray with user testing.
#2796597: Validate usability of visual design in outside-in off canvas tray with user testing
Comment #36
tim.plunkettComment #37
tkoleary CreditAttribution: tkoleary at Acquia commentedChanges look like this:
Comment #38
SKAUGHTjust to comment: the addition of being able to drag the tray wider/narrower is very nice.
Comment #39
tkoleary CreditAttribution: tkoleary at Acquia commented@skaught
We got that for free from jquery UI AFAICT.
Comment #40
Bojhan CreditAttribution: Bojhan as a volunteer and commented@tkoleary Alright, thats not really in line with our other styles (e.g. the toolbar we use the lighter color to emphasize, instead of the dark). However given the major improvement this already is, that nitpick is not important. I doubt we can really "test" the visual design, you test the "usability" and occasionally visual design is an artefact of that.
Comment #41
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
We can totally test visual design independent of general usability. Al we need to do is have a control. In this case the control would be the module with it's theme css turned off so that it just inherits from jquery UI.
Coupling that with a set of tasks and questions that probe clarity, discoverability, and completion time should tell us a lot of things.
Comment #42
Bojhan CreditAttribution: Bojhan as a volunteer and commentedWell we could :) But given that we rarely test, I would question it's impact vs. other tests.
Comment #43
tkoleary CreditAttribution: tkoleary at Acquia commentedSome work to make the file structure more clear and files more well commented.
Comment #44
tkoleary CreditAttribution: tkoleary at Acquia commentedIgnore the interdiff in 43, this is the correct one.
Comment #45
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #46
tkoleary CreditAttribution: tkoleary at Acquia commentedCopied relevant styles from details component and altered.
Copied relevant styles from form component and altered.
todo done
Copied relevant styles from table component and altered.
Copied relevant styles from tabledrag component and altered.
There is a child of this issue just for messages
Copied from quickedit module so button colors are consistent
This never worked
Added after testing across multiple themes and seeing a lot of 'bleedthrough' of theme styles
These should be moved into a global reset css for jquery styles.
This chunk was moved to details component
Moved to table component
Scoped this to all tables in the tray
237-310 moved into forms component
todo done
todo done
CSS inherited from dropbutton that's overly complex and could be accomplished with an icon. Should be changed upstream in dropbutton.
Grouped toolbar related styles
Added components
Comment #47
rickvug CreditAttribution: rickvug commentedI haven't applied the patch to test but based on the screenshot in #22 I'm wondering if the default width should be slightly wider. Look at how "Recent Content (disabled)" wraps to 3 lines. I'd expect there to be a lot more excess line wrapping in the real world. The fact that the drawer is already resizable is a big win.
My other nitpick is that the submit buttons look a few px too narrow. Overall this is looking great. Huge improvement.
Comment #48
tkoleary CreditAttribution: tkoleary at Acquia commented@rickvug
Thanks!
In the latest patch table cells are set to whitespace nowrap with overflow ellipsis. We're also hiding the descriptions and the font size is slightly smaller so everything fits better.
Comment #49
Bojhan CreditAttribution: Bojhan as a volunteer and commented@tkoleary Should the select list be full width too?
Comment #50
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI would really like to get this in before release, I think it will have a big impact on adoption and appreciation of this module.
Can we devise a strategy to get it in sooner?
Comment #51
tkoleary CreditAttribution: tkoleary at Acquia commentedIt could, but then we'd need to do a lot of work on individual styling of all of the 'text type' selects like number, date etc. If we just made .form-select 100% all of those would be 100% as well (AFAICT). I think that's a good future enhancement though.
As far as I understand this can be committed to 8.3 and backported to 8.2 immediately since it does not target any existing markup or break backwards compatibility. Please do mark it RTBC though, if you're happy with it, and we can make it happen.
Comment #52
Bojhan CreditAttribution: Bojhan as a volunteer and commentedWe should do that in a future improvement.
Definitively happy with it, lets do it! It's experimental so shouldn't need thorough review on changes to CSS.
Comment #54
tkoleary CreditAttribution: tkoleary at Acquia commentedRerolling patch
Comment #55
nod_Comment #56
tkoleary CreditAttribution: tkoleary at Acquia commentedNo changes, just pulled and rebased against core.
Comment #57
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #58
tkoleary CreditAttribution: tkoleary at Acquia commentedFound one visual regression when I tested on simplytest.me. Rerolled again.
These focus effects were too broadly scoped and are handled in theme css.
This was set to #fff when the tray background was #fff. Changing to #333 to match new tray style.
Comment #59
rickvug CreditAttribution: rickvug commented@tkoleary The changes address my concern. This is looking great!
Comment #60
tkoleary CreditAttribution: tkoleary at Acquia commented@rickvug,
Thanks! Moving back to RTBC since it's passing tests now.
Comment #62
webchickYAY! This looks SO much better!! So glad to get this in before 8.2.0's release.
Committed and pushed to 8.2.x and 8.3.x.
Comment #65
Gábor HojtsyComment #66
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)