feat(side-sheet): Create side-sheet package #3924
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/side-sheet #3924 +/- ##
================================================
Coverage 98.52% 98.52%
================================================
Files 120 120
Lines 5224 5224
Branches 657 657
================================================
Hits 5147 5147
Misses 77 77Continue to review full report at Codecov.
|
|
🤖 Beep boop! Screenshot test report 🚦18 screenshots changed from |
packages/mdc-side-sheet/README.md
Outdated
|
|
||
| The MDC Side Sheet is used to organize access to destinations and other functionality on an app. |
There was a problem hiding this comment.
Based on material.io wording:
The MDC Side Sheet is a supplementary surface primarily used on tablet and desktop.
packages/mdc-side-sheet/README.md
Outdated
| ```html | ||
| <aside class="mdc-side-sheet"> | ||
| <div class="mdc-side-sheet__content"> | ||
| <div><label><input type="checkbox"> Events</label></div> |
There was a problem hiding this comment.
Leave the inner contents out, just put ..., since we have no opinion on the internals (but what's here now isn't MD-ified at all).
packages/mdc-side-sheet/README.md
Outdated
| </aside> | ||
| ``` | ||
|
|
||
| #### Menu Icon |
There was a problem hiding this comment.
Genericize this to just "Icon" and add a leading sentence such as the following:
Side sheets are typically dismissible or modal, and are opened via an affordance such as an icon in a Top App Bar.
packages/mdc-side-sheet/README.md
Outdated
|
|
||
| ### Dismissible Side Sheet | ||
|
|
||
| Dismissible side sheets are by default hidden off screen, and can slide into view. Dismissible side sheets should be used when navigation is not common, and the main app content is prioritized. |
There was a problem hiding this comment.
Remove the second sentence, which is drawer-specific
packages/mdc-side-sheet/README.md
Outdated
| <body> | ||
| <aside class="mdc-side-sheet mdc-side-sheet--dismissible"> | ||
| <div class="mdc-side-sheet__content"> | ||
| <div><label><input type="checkbox"> Events</label></div> |
There was a problem hiding this comment.
Replace internals with ...
packages/mdc-side-sheet/README.md
Outdated
|
|
||
| #### Dismissible Side Sheet | ||
|
|
||
| Restore focus to the first focusable element when a list item is activated or after the side-sheet closes. Do not close the side-sheet upon item activation, since it should be up to the user when to show/hide the dismissible side-sheet. |
There was a problem hiding this comment.
This is left over from drawer and needs to be updated or removed. Check if there's particular guidance RE whether anything aside from an optional close affordance should close the side sheet (and I'm not sure a close affordance is required since you can toggle it via the same icon you opened it with?)
packages/mdc-side-sheet/README.md
Outdated
|
|
||
| #### Modal Side Sheet | ||
|
|
||
| Close the side-sheet when an item is activated in order to dismiss the modal as soon as the user performs an action. Only restore focus to the first focusable element in the main content after the side-sheet is closed, since it's being closed automatically. |
There was a problem hiding this comment.
Ditto previous comment RE drawer-specific leftovers
packages/mdc-side-sheet/README.md
Outdated
|
|
||
| ## Usage within Web Frameworks | ||
|
|
||
| If you are using a JavaScript framework, such as React or Angular, you can create a Navigation Side Sheet for your framework. Depending on your needs, you can use the _Simple Approach: Wrapping MDC Web Vanilla Components_, or the _Advanced Approach: Using Foundations and Adapters_. Please follow the instructions [here](../../docs/integrating-into-frameworks.md). |
packages/mdc-side-sheet/_mixins.scss
Outdated
| @mixin mdc-side-sheet-ink-color($color, $opacity: $mdc-side-sheet-ink-opacity) { | ||
| $value: rgba(mdc-theme-prop-value($color), $opacity); | ||
|
|
||
| .mdc-side-sheet__title { |
There was a problem hiding this comment.
This selector shouldn't be here, right? (This is probably why I thought the accessible fill color test page looked odd, and we don't seem to have a test page for ink color?)
|
Now I remember why I originally kept
As a result, the same content will be laid out differently depending on whether it's inside a We could potentially make It seems like the simplest option is to keep the |
|
🤖 Beep boop! Screenshot test report 🚦30 screenshots changed from Details |
|
🤖 Beep boop! Screenshot test report 🚦30 screenshots changed from Details |
|
🤖 Beep boop! Screenshot test report 🚦30 screenshots changed from Details |
…nd modal Sass files
| display: flex; | ||
| flex-direction: column; | ||
| flex-shrink: 0; | ||
| order: $flexOrder; |
There was a problem hiding this comment.
Am I right to assume this is compensating for the markup being before the app content element for the top app bar example, where the parent is display: flex?
Also, can we even be sure that 1 is the right value for this? If you have a drawer, app content, and side sheet, does this still work?
(Hopefully we'll be able to remove this if we eventually come up with a solution which avoids needing to put trailing side sheet before app content.)
There was a problem hiding this comment.
Yes, that's correct. I added a comment in the Sass to clarify this.
I manually tested drawer + app content + side sheet, and 1 seemed to work fine. According to the Chrome Dev Tools, the default order for flex items is 0, so 1 will always "win".
Should I create an issue to track finding a better solution that doesn't require a specific source order?
There was a problem hiding this comment.
Yeah, I haven't gotten around to adding one but we need to, in terms of finding something that doesn't present problems for tab order as well as flex order.
There was a problem hiding this comment.
Created #3944 for this 🙂
packages/mdc-side-sheet/_mixins.scss
Outdated
| transition-timing-function: $mdc-animation-standard-curve-timing-function; | ||
| overflow: hidden; | ||
|
|
||
| #{&}--open { |
There was a problem hiding this comment.
Why did we add explicit interpolation here?
I did a quick test with sassmeister...and then didn't believe my eyes so I tested node-sass, dart-sass, AND ruby sass locally... but doing this seems to cause redundant selectors in all of them, for some reason.
Example:
@mixin foo {
&--dismissible {
...
}
}
.mdc-drawer {
@include foo;
}As written above, this yields .mdc-drawer--dismissible { ... }.
With #{&}--dismissible instead, it yields .mdc-drawer .mdc-drawer--dismissible { ... }.
There was a problem hiding this comment.
I originally had &, but stylelint threw a fit:
Expected "&--open" to have no more than 0 type selectors
Strangely, it only complains about &--open, but not any of the other similar selectors
(&--animate, &--opening, or &--closing). It must be a bug in stylelint.
I reverted back to & and added a stylelint-disable-next-line selector-max-type comment to suppress the linter error.
Thanks!
|
|
||
| @include mdc-rtl { | ||
| transform: translateX(100% * $xFactor); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @mixin mdc-side-sheet-modal_($side: trailing) { | ||
| @mixin mdc-side-sheet-dismissible_($rootClass, $side: trailing) { |
There was a problem hiding this comment.
Is $rootClass avoidable for these mixins if we operate under the assumption that this is invoked in the context of the component's root class? i.e. then &.#{$rootClass}--open becomes &--dismissible#{&}--open? (Interpolation is necessary for the second one, but not the first, per my other comment.)
There was a problem hiding this comment.
It's possible, yes, but it feels a little weird to me: 4d73a7a
Specifically, mdc-side-sheet.scss ends up looking like this:
.mdc-side-sheet {
@include mdc-side-sheet-base_;
@include mdc-side-sheet-dismissible_;
@include mdc-side-sheet-modal_;
@include mdc-side-sheet-scrim_;
@include mdc-side-sheet-stroke-width_(1px);
@include mdc-side-sheet-stroke-color($mdc-side-sheet-stroke-color);
@include mdc-side-sheet-surface-fill-color($mdc-side-sheet-surface-fill-color);
// ...
}
.mdc-side-sheet--modal {
@include mdc-side-sheet-scrim-fill-color($mdc-side-sheet-modal-scrim-color);
@include mdc-elevation($mdc-side-sheet-modal-elevation);
}-
The root CSS class calls mixins for itself and the two variants and the scrim, which feels odd.
-
The root CSS class calls a
modal_mixin, but right below it, there's also a--modalCSS class. I could move those two mixin calls intomdc-side-sheet-modal_, but we normally try to keep all the theme configuration in the mainmdc-foo.scssfile. -
It forces most of the CSS classes to go in mixins instead of the main
mdc-side-sheet.scssfile, which IMO makes the styles harder to read and understand.- On a related note,
&--openselectors make search/replace harder, and it's easy to forget about them when you're renaming something.
- On a related note,
The $rootClass param isn't great, but it seems cleaner than the alternative. And the mixins that have it are all "private", so we don't need to worry about clients using it.
WDYT?
There was a problem hiding this comment.
My original concern was mostly just the required repetition of the base class which we could easily mess up in copypasta, but I guess that's a pretty small price to pay which should also be obvious in screenshot tests. So I'm willing to go with that since this didn't pan out.
| this.closed(); | ||
| this.adapter_.notifyClose(); | ||
| } else { | ||
| this.adapter_.focusActiveNavigationItem(); |
There was a problem hiding this comment.
I'm not talking about the trap/restore focus APIs. I'm talking about the "focus active navigation item" API, which is drawer-specific, and typically won't actually do anything for side sheet (unless it just happens to have a MDC List with an activated item.
We probably do still want to focus something within the dismissible side sheet when it opens...but we'd probably have to just look for the first focusable thing within it. I forget if we've already implemented or used something like that somewhere else in our repo? Otherwise we could probably use tabbable (focus-trap's dependency) for it, but might want to delegate that to a util function as well.
We can split this to another task if it makes it clearer to leave this as it was from drawer in this first PR.
|
🤖 Beep boop! Screenshot test report 🚦30 screenshots changed from Details |
packages/mdc-side-sheet/package.json
Outdated
| "@material/animation": "^0.40.1", | ||
| "@material/base": "^0.40.1", | ||
| "@material/elevation": "^0.40.1", | ||
| "@material/list": "^0.40.1", |
There was a problem hiding this comment.
I don't think side sheet needs list or ripple?
|
🤖 Beep boop! Screenshot test report 🚦30 screenshots changed from Details |
|
🤖 Beep boop! Screenshot test report 🚦30 screenshots changed from Details |
acdvorak
changed the title
I copied the
mdc-drawerpackage, renamed it, and deleted all drawer-specific code.Applicable mixins have a
$sideparam that defaults totrailing, and can be overridden toleadingfor drawer.Related issues:
mdc-side-sheetcomponent #2665TODO:
padding: 16pxon contentmdc-typography-base()mixin for contentmdc-side-sheetclass.Follow up PRs: