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

fix(expansion): define default expansion header heights via css. #9313

Merged
merged 1 commit into from
Jan 23, 2018
Merged

fix(expansion): define default expansion header heights via css. #9313

merged 1 commit into from
Jan 23, 2018

Conversation

josephperrott
Copy link
Member

Addresses: #8592

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Jan 9, 2018
@josephperrott
Copy link
Member Author

Fixed author to be correct email for CLA.

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jan 9, 2018
@josephperrott
Copy link
Member Author

@jelbourn @crisbeto

I think this might be a better way to go about allowing custom heights for expansion panel headers as it will allow customization using either css or binding in the template. I want to confirm that there is not something I am missing that would cause issues here.

@angular angular deleted a comment from googlebot Jan 10, 2018
jelbourn
jelbourn approved these changes Jan 10, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, @crisbeto may have input

crisbeto
crisbeto reviewed Jan 10, 2018
View reviewed changes
src/lib/expansion/expansion-animations.ts
}),
state('expanded', style({
height: '{{expandedHeight}}'
}), {
params: {expandedHeight: '64px'}
params: {expandedHeight: '*'}
Copy link
Member

Choose a reason for hiding this comment

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

I agree that it's a better way to handle custom height, but AFAIK it will break the people that are using the binding as it's set up right now. We could either remove the inputs and leave these changes for a later release, or deprecate them and set up the bindings so they just set the height as an inline style.

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't break those using it as it is currently since we define the same expanded and collapsed heights (64px and 48px) in css rather than the animation parameters.

If a user set it by changing the animation params, it should be respected as it will be put into the style tag and override our css rule's height.
If a user set it by setting the params to * and defined the css rule's, then it results as the same how the new set up works.

Am I missing a scenario that would break?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I probably got confused last time. Looking at it again, it should be fine.

crisbeto
crisbeto approved these changes Jan 12, 2018
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.

LGTM

src/lib/expansion/expansion-animations.ts
}),
state('expanded', style({
height: '{{expandedHeight}}'
}), {
params: {expandedHeight: '64px'}
params: {expandedHeight: '*'}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I probably got confused last time. Looking at it again, it should be fine.

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 12, 2018
@jelbourn jelbourn merged commit c604834 into angular:master Jan 23, 2018
@josephperrott josephperrott mentioned this pull request Jan 24, 2018
Closed
josephperrott added a commit to josephperrott/components that referenced this pull request Jan 24, 2018
@josephperrott
chore(expansion): revert pull angular#9313
33b273c
jelbourn pushed a commit that referenced this pull request Jan 25, 2018
@josephperrott @jelbourn
chore(expansion): revert pull #9313 (#9580)
b3a0de9
@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Jan 29, 2018
@iprins iprins mentioned this pull request Feb 27, 2018
Closed
@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 Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto approved these changes

@jelbourn jelbourn jelbourn approved these changes

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 target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@josephperrott @googlebot @jelbourn @crisbeto