Skip to content

fix(dialog) make align attribute into an input of dialog actions directive instead of css #21439

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
Closed
wants to merge 5 commits into from
Closed

fix(dialog) make align attribute into an input of dialog actions directive instead of css #21439

wants to merge 5 commits into from

Conversation

PooSham
Copy link
Contributor

@PooSham PooSham commented Dec 25, 2020

Fixes multiple issues, such as self-documentation of the align attribute, type checking, better bindings, and IDE support. Because align is not standard for HTMLDivElement, it doesn't make much sense to assume end users to know they can use the align attribute.

Fixes #18479

@PooSham
fix(material/dialog): make align attribute into an input of dialog ac…
c101f7f 
…tions directive instead of css

Fixes multiple issues, such as self-documentation of the align attribute, type checking, better
bindings, and IDE support. Because align is not standard for HTMLDivElement, it doesn't make much
sense to assume end users to know they can use the align attribute.

Fixes angular#18479
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 25, 2020
@PooSham PooSham force-pushed the align-attr-as-input-in-dialog-actions-#18479 branch 3 times, most recently from 7b51418 to 5da4caa Compare December 28, 2020 15:23
@PooSham
Copy link
Contributor Author

PooSham commented Dec 28, 2020

I can't figure out how to add target label, I assume I don't have the rights to do it. It should probably be a patch.

crisbeto
crisbeto reviewed Jan 4, 2021
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

I believe that this will be a breaking change for people using [attr.align]="someExpression". Also it looks like a bunch of unrelated code was modified.

@PooSham PooSham closed this Jan 4, 2021
@PooSham PooSham reopened this Jan 4, 2021
@PooSham
Copy link
Contributor Author

PooSham commented Jan 4, 2021

@crisbeto

I changed both in material-experimental and the regular material, including tests and example files. The formatting changed in some files as well automatically by clang-format (which is a recommended extension to VS Code). If you want, I can revert those changes.

Regarding the [attr.align]="someExpression" breaking, that is true for material-experimental, but not for the regular material. I will fix the issue in material-experimental. I do believe it should be broken at one point since align isn't a standard HTML attribute, but I guess that's not up to me to decide.

@PooSham
fix(material-experimental/mdc-dialog): make align attr into an input …
462d407 
…of dialog actions directive

Fixes multiple issues, such as self-documentation of the align attribute, type checking, better
bindings, and IDE support. Because align is not standard for HTMLDivElement, it doesn't make much
sense to assume end users to know they can use the align attribute.

Fixes angular#18479 for material-experimental
@PooSham PooSham force-pushed the align-attr-as-input-in-dialog-actions-#18479 branch from 1a05339 to 462d407 Compare January 4, 2021 14:35
@PooSham PooSham requested a review from crisbeto January 5, 2021 15:29
@devversion devversion removed their request for review August 18, 2021 13:06
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@PooSham
Copy link
Contributor Author

PooSham commented Jan 3, 2022

@crisbeto @jelbourn Over a year in the making, will this be merged? :)

@PooSham
Copy link
Contributor Author

PooSham commented Jan 5, 2022

I had configured a new email account in git, but it wasn't added to the CLA. It should be fixed now, could a repository manager please rerun the check? @crisbeto @jelbourn

@PooSham
Copy link
Contributor Author

PooSham commented Jan 7, 2022

@crisbeto Can you please rerun the checks now that I've updated the CLA?

@crisbeto
Copy link
Member

crisbeto commented Jan 8, 2022

There's a bug in CircleCI that sometimes prevents the workflows from running and we don't have a way of triggering them. The one above which says that it needs approval is for deploying the dev app and it is optional.

@PooSham
Copy link
Contributor Author

PooSham commented Jan 8, 2022

There's a bug in CircleCI that sometimes prevents the workflows from running and we don't have a way of triggering them. The one above which says that it needs approval is for deploying the dev app and it is optional.

Oh ok, seems to have run now. Do you have any feedback on the PR? Is there anything that needs to be fixed?

crisbeto
crisbeto reviewed Jan 9, 2022
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

It looks like CircleCI still didn't run. It might be easier to resubmit the PR.

src/material/dialog/dialog-content-directives.ts
})
export class MatDialogActions {}
export class MatDialogActions {
@Input() align?: 'center' | 'end' = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than undefined, should it support start which doesn't apply any styling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will resubmit a new PR.

Yes, that might be better for API consistency. I will do that change in my next PR

@PooSham PooSham mentioned this pull request Jan 9, 2022
Closed
@PooSham
Copy link
Contributor Author

PooSham commented Jan 9, 2022

New PR here. Hope it works this time...

@crisbeto

@PooSham PooSham closed this Jan 9, 2022
@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 Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@jelbourn jelbourn Awaiting requested review from jelbourn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clarify use of non-standard "align" attribute for mat-dialog-actions
3 participants
@PooSham @crisbeto @andrewseguin