feat(dialog): Reverse buttons when stacked; allow toggling auto-stack #3573
Conversation
kfranqueiro
requested review from
abhiomkar
and removed request for
acdvorak
| dialog.autoStackButtons = false; | ||
| ``` | ||
|
|
||
| This will also be disabled if the `mdc-dialog--stacked` modifier class is applied manually to the root element before the |
There was a problem hiding this comment.
is this implemented in foundation? couldn't find logic which detects the existence of --stacked modifier class before auto stack.
There was a problem hiding this comment.
It's currently implemented in the component (where you left another comment), but I'm moving it to the foundation.
|
|
||
| if (areButtonsStacked !== this.areButtonsStacked_) { | ||
| this.adapter_.reverseButtons(); | ||
| this.areButtonsStacked_ = areButtonsStacked; |
There was a problem hiding this comment.
This assignment can be outside if condition.
There was a problem hiding this comment.
If it's outside the if, it's going to assign the value even when it's already the same value. Putting it inside the if makes it only assign the value when it's actually going to do something.
There was a problem hiding this comment.
Yes, but I think we don't have to condition this since it is not toggling as in .reverseButton(). But this is fine too.
packages/mdc-dialog/index.js
Outdated
| @@ -132,6 +140,10 @@ class MDCDialog extends MDCComponent { | |||
| this.listen('click', this.handleClick_); | |||
| this.listen(strings.OPENING_EVENT, this.handleOpening_); | |||
| this.listen(strings.CLOSING_EVENT, this.handleClosing_); | |||
|
|
|||
| if (this.root_.classList.contains(MDCDialogFoundation.cssClasses.STACKED)) { | |||
There was a problem hiding this comment.
Should this logic be in foundation?
There was a problem hiding this comment.
Originally I put this in initialSyncWithDOM thinking that foundation.init could be too early for frameworks, but (1) I realized MDC React only calls foundation.init within componentDidMount anyway, and (2) I noticed we're doing stuff that's root-class-dependent even as soon as components are constructed (see drawer and top app bar's getDefaultFoundation), so if that doesn't cause a problem then this definitely won't.
I've moved it to foundation#init (required adding another adapter API).
|
|
||
| if (areButtonsStacked !== this.areButtonsStacked_) { | ||
| this.adapter_.reverseButtons(); | ||
| this.areButtonsStacked_ = areButtonsStacked; |
There was a problem hiding this comment.
Yes, but I think we don't have to condition this since it is not toggling as in .reverseButton(). But this is fine too.
Codecov Report
@@ Coverage Diff @@
## master #3573 +/- ##
==========================================
- Coverage 98.42% 98.35% -0.08%
==========================================
Files 120 120
Lines 5133 5157 +24
Branches 637 640 +3
==========================================
+ Hits 5052 5072 +20
- Misses 81 85 +4
Continue to review full report at Codecov.
|
Fixes #3561.
BREAKING CHANGE: Adds hasClass and reverseButtons adapter APIs.