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.

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

fix(list): properly align contents in subheader #6221

Merged

Conversation

devversion
Copy link
Member

  • The line-height of the subheaders is currently based on the typography level. This is problematic because the subheader is set to a specific height and can't grow/shrink accordingly.

Fixes #6214

* The line-height of the subheaders is currently based on the typography level. This is problematic because the subheader is set to a specific height and can't grow/shrink accordingly.

Fixes angular#6214
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 2, 2017
@@ -46,7 +46,8 @@
}

.mat-subheader {
@include mat-typography-level-to-styles($config, body-2);
font: mat-font-weight($config, body-2) mat-font-size($config, body-2)
Copy link
Member

Choose a reason for hiding this comment

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

The font declaration is a little fiddly, especially when omitting values. I'd rather have this split into font-weight, font-size and font-family.

Copy link
Member Author

@devversion devversion Aug 2, 2017

Choose a reason for hiding this comment

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

The font property is used in this file for the dense mode as well (see here).

I'm fine changing it for both if desired.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that. Also the font-family could be set once on the .mat-list and the subheader can inherit from there instead.

Copy link
Member Author

@devversion devversion Aug 2, 2017

Choose a reason for hiding this comment

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

Yeah makes sense.

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

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Aug 2, 2017
@kara kara removed their assignment Aug 2, 2017
@grizzm0
Copy link
Contributor

grizzm0 commented Aug 2, 2017

What about the dense attribute I also pointed out? Should I create a separate issue for that? Right now the same thing would happen in dense mode as there's only 8px left for the text. The spec doesn't even mention a dense subheader in dense mode.

@devversion
Copy link
Member Author

devversion commented Aug 2, 2017

@grizzm0 This fix should also calculate the proper line-height for subheaders in dense mode.

@grizzm0
Copy link
Contributor

grizzm0 commented Aug 2, 2017

@devversion Yeah, but the height of subheader in dense mode is changed from 48px to 40px. (even tho the spec doesn't mention it).

The padding is still 2x16px = 32px. Meaning there's only 8px left for the text while the font-size in this PR is caption (12px). That's 4px overflow which the text will be "pushed down" in dense mode.

Edit: Ok, I see what you mean with the line-height. It will still "stick out" 2px on the top and 2px on the bottom tho. As you're trying to fit a 12px/8px font into an 8px high space. Hence me saying we're not supposed to change the height from 48px to 40px in dense mode in the subheader. Only on the items themselfs.

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

@tinayuangao tinayuangao merged commit 4e6e42e into angular:master Aug 2, 2017
@devversion devversion deleted the fix/list-align-subheader-text branch November 11, 2017 10:22
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subheader line-height is too high
8 participants