Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(material-experimental/mdc-button): add extended fab #21585

Merged
merged 4 commits into from
Feb 5, 2021
Merged

feat(material-experimental/mdc-button): add extended fab #21585

merged 4 commits into from
Feb 5, 2021

Conversation

annieyw
Copy link
Contributor

@annieyw annieyw commented Jan 14, 2021

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 14, 2021
@annieyw
Copy link
Contributor Author

annieyw commented Jan 14, 2021

I still need to fix the unit tests but wanted to see if the inputs/classes should be added this way

crisbeto
crisbeto reviewed Jan 14, 2021
View reviewed changes
src/material-experimental/mdc-button/fab.ts Outdated
@@ -49,6 +52,8 @@ export class MatFabButton extends MatButtonBase {
// The FAB by default has its color set to accent.
color = 'accent' as ThemePalette;

extended: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "extended" the only variation or do we expect more? If we expect more, we can turn it into a string instead, e.g. appearance: 'extended'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be the only variation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make this a setter/getter and use coerceBooleanInput that way peopel can do <button mat-fab extended> instead of <button mat-fab [extended]="true"> (coerceBooleanInput does some non-standard type coercion, e.g. '' => true)

src/material-experimental/mdc-button/fab.ts Outdated Show resolved Hide resolved
@annieyw
Copy link
Contributor Author

annieyw commented Jan 15, 2021

Addressed comments and split up mat-fab and mat-mini-fab since the extended input should only be available for the normal mat-fab

jelbourn
jelbourn reviewed Jan 19, 2021
View reviewed changes
src/material-experimental/mdc-button/button.spec.ts Outdated
// expect(extendedFabButtonDebugEl.nativeElement.classList)
// .toContain('mat-accent', 'Expected extended fab buttons to use accent palette by default');
// });
// });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented block?

src/material-experimental/mdc-button/fab.ts Outdated
@@ -49,27 +63,76 @@ export class MatFabButton extends MatButtonBase {
// The FAB by default has its color set to accent.
color = 'accent' as ThemePalette;

private _extended: boolean;
get extended() { return this._extended; }
set extended(extended: any) { this._extended = coerceBooleanProperty(extended); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type here should still be boolean, but the class should define an ngAcceptInputType to BooleanInput (search for other ngAcceptInputType instances for examples)

src/material-experimental/mdc-button/fab.ts
*/
@Component({
selector: `button[mat-mini-fab]`,
templateUrl: 'button.html',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, a thought occurred to me that having multiple components with the same template would lead to duplicate generated code. I checked with Andrew K, and he confirmed. So, rather than introducing a separate component with the same template, we should probably try to have one class with the templateUrl as the base class and then extend all the other buttons from it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all buttons extend MatButtonBase with the same template (and anchos MatAnchorBase). Should there be another MatFabBase?

@annieyw annieyw force-pushed the extened-fab branch 2 times, most recently from 1afd252 to 23caa44 Compare January 20, 2021 03:07
mmalerba
mmalerba approved these changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/material-experimental/mdc-button/fab.ts Outdated
@@ -78,9 +141,39 @@ export class MatFabAnchor extends MatAnchor {
// The FAB by default has its color set to accent.
color = 'accent' as ThemePalette;

extended: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one too

@annieyw annieyw added G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release action: merge The PR is ready for merge by the caretaker labels Jan 22, 2021
@annieyw annieyw merged commit f7ace54 into angular:master Feb 5, 2021
annieyw added a commit that referenced this pull request Feb 5, 2021
@annieyw
feat(material-experimental/mdc-button): add extended fab (#21585)
ea7bdda 
* feat(material-experimental/mdc-button): add extended fab

* fix(material-experimental/mdc-button): split up fab and mini fab

* fix(material-experimental/mdc-button): fix icon styling and add example

* fix(material-experimental/mdc-button): add getter/setter for extended property

(cherry picked from commit f7ace54)
annieyw added a commit that referenced this pull request Feb 5, 2021
@annieyw
feat(material-experimental/mdc-button): add extended fab (#21585)
08a8fe0 
* feat(material-experimental/mdc-button): add extended fab

* fix(material-experimental/mdc-button): split up fab and mini fab

* fix(material-experimental/mdc-button): fix icon styling and add example

* fix(material-experimental/mdc-button): add getter/setter for extended property

(cherry picked from commit f7ace54)
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Feb 8, 2021
@annieyw @wagnermaciel
feat(material-experimental/mdc-button): add extended fab (angular#21585)
1f28890 
* feat(material-experimental/mdc-button): add extended fab

* fix(material-experimental/mdc-button): split up fab and mini fab

* fix(material-experimental/mdc-button): fix icon styling and add example

* fix(material-experimental/mdc-button): add getter/setter for extended property
wagnermaciel pushed a commit to wagnermaciel/components that referenced this pull request Feb 8, 2021
@annieyw @wagnermaciel
feat(material-experimental/mdc-button): add extended fab (angular#21585)
715a491 
* feat(material-experimental/mdc-button): add extended fab

* fix(material-experimental/mdc-button): split up fab and mini fab

* fix(material-experimental/mdc-button): fix icon styling and add example

* fix(material-experimental/mdc-button): add getter/setter for extended property
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@crisbeto crisbeto crisbeto left review comments

@mmalerba mmalerba mmalerba approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@annieyw @jelbourn @crisbeto @mmalerba