feat(button): create stroked and flat button variants #9365
Conversation
googlebot
added
the
cla: yes
src/lib/button/_button-base.scss
Outdated
| // Applies styles to buttons with backgrounds: raised, fab, and mini-fab | ||
| %mat-raised-button { | ||
| // Applies styles to buttons with stroke styling | ||
| %mat-stroked-button { |
There was a problem hiding this comment.
Why make stroked and flat sass placeholders? The raised button was a placeholder because the styles applied for both regular raised for fabs.
src/lib/button/_button-base.scss
Outdated
| @include mat-overridable-elevation(2); | ||
| @include mat-overridable-elevation(0); | ||
|
|
||
| outline: 1px solid currentColor; |
There was a problem hiding this comment.
Can this be done with border instead of outline? Generally outline should be avoided since things its used by the browser for focus treatment, devtools for element selection, etc.
There was a problem hiding this comment.
Done, it was initially done this way to prevent the border from changing the offset height/width of the button.
src/lib/button/_button-theme.scss
Outdated
| @@ -99,6 +99,23 @@ | |||
| @include _mat-button-ripple-color($theme, default, 0.1); | |||
| } | |||
|
|
|||
| .mat-stroked-button { | |||
| color: rgba(0, 0, 0, 0.87); | |||
There was a problem hiding this comment.
This should probably be pulled from the foreground palette (which accounts for light vs. dark theme)
src/lib/button/button.ts
Outdated
| selector: 'button[mat-flat-button], a[mat-flat-button]', | ||
| host: {'class': 'mat-flat-button'} | ||
| }) | ||
| export class MatFlatButtonCssMatStyler {} |
There was a problem hiding this comment.
These separate styler directives aren't really necessary; now would be a good time to do away with them here. You can have MatButton just apply the right class checking the attributes (i.e. adding a new method like _setButtonVariantClass)
josephperrott
force-pushed
the
button
branch
2 times, most recently
from
23f96c8 to
aabf57e
Compare
src/lib/button/button.ts
Outdated
| '[class.mat-raised-button]': `_hasHostAttributes('mat-raised-button')`, | ||
| '[class.mat-stroked-button]': `_hasHostAttributes('mat-stroked-button')`, | ||
| '[class.mat-mini-fab]': `_hasHostAttributes('mat-mini-fab')`, | ||
| '[class.mat-fab]': `_hasHostAttributes('mat-fab')`, |
There was a problem hiding this comment.
It's a bit overkill to add 7 bindings when we know that only one of these is going to apply (the bindings will generate more code and incur higher update costs). We only really need to check once at constructor time and add the class manually.
josephperrott
force-pushed
the
button
branch
4 times, most recently
from
1b11ccd to
245a4f2
Compare
jelbourn
added
pr: lgtm
action: merge
josephperrott
force-pushed
the
button
branch
2 times, most recently
from
21dd83b to
a863e75
Compare
|
@josephperrott please rebase |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.