-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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(material/list): list item truncates on narrow screen widths #29033
base: main
Are you sure you want to change the base?
fix(material/list): list item truncates on narrow screen widths #29033
Conversation
Deployed dev-app for 369bb8c to: https://ng-dev-previews-comp--pr-angular-components-29033-dev-eoyhezyg.web.app Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt. |
src/material/list/list.scss
Outdated
line-height: normal; | ||
} | ||
.mat-mdc-list-item-title.mdc-list-item__primary-text { | ||
height: 45px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is added. It's moving the secondary text closer to the primary text, and I don't see how it improves readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andrewseguin! Removed previous attempt to improve readability and revised by increasing the list items with 3 lines from 88px to min-height 90px so that the bottom letters of the 3rd line do not get cut off. See screenshots for reference:
- Before screenshot
- After screenshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the heights won't work if the text needs to wrap to several lines. Try adding more secondary text to see that it only shows two lines.
Ideally the text container grows as necessary and doesn't set height at all. This might require making more significant changes to the list item styles
For example, we might want to get rid of height: var(--mdc-list-list-item-three-line-container-height);
altogether
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andrewseguin! Thanks for the rec. I did some further digging and it looks like that variable (value of 88px) is coming from _list.js theming for density (screenshot here) . Would it be better to override the value by adding in a height: auto
in the list.scss
or remove the specific values from the theming? Honestly I haven't worked directly with affecting overall theming so wanted to check before moving in that direction.
d12043b
to
9e08fcc
Compare
1d9b95a
to
4023349
Compare
dff207f
to
91b2390
Compare
src/material/list/list.scss
Outdated
.mat-mdc-list-item.mdc-list-item--with-leading-checkbox.mdc-list-item--with-three-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-icon.mdc-list-item--with-three-lines { | ||
height: auto; | ||
padding-bottom: 12px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to 16px
to match previous padding
src/material/list/list.scss
Outdated
// if the list item has acquired three lines. We unset these styles for line elements. | ||
// https://github.com/material-components/material-components-web/blob/348665978ce73694ad4518626dd70cdf5b984113/packages/mdc-list/_evolution-mixins.scss#L205-L206. | ||
// TODO: Consider removing once MDC supports the explicit tertiary line list variant. | ||
.mat-mdc-list-item.mdc-list-item--with-leading-avatar.mdc-list-item--with-two-lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this helps center it if the text wrap to two lines, but fails if its more. I'd just take this out
91b2390
to
c1a0a27
Compare
src/material/list/list.scss
Outdated
.mat-mdc-list-item.mdc-list-item--with-leading-checkbox.mdc-list-item--with-three-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-icon.mdc-list-item--with-three-lines { | ||
height: auto; | ||
padding-bottom: 16px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 16px
looks good, but 8px
will match the previous style and probably produce far less screenshot test failures
// Increased list item height if it has multiple lines so bottom line doesn't get | ||
// cut off. Also added padding-bottom so there's space btw the text and the divider. | ||
.mat-mdc-list-item.mdc-list-item--with-leading-avatar.mdc-list-item--with-two-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-checkbox.mdc-list-item--with-two-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-icon.mdc-list-item--with-two-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-avatar.mdc-list-item--with-three-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-checkbox.mdc-list-item--with-three-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-leading-icon.mdc-list-item--with-three-lines { | ||
height: auto; | ||
padding-bottom: 8px; | ||
} | ||
|
||
.mat-mdc-list-item.mdc-list-item--with-three-lines, | ||
.mat-mdc-list-item.mdc-list-item--with-two-lines { | ||
height: auto; | ||
padding-bottom: 13px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized we can make this a little cleaner by nesting the special case right under where you set the padding-bottom:
// Increased list item height if it has multiple lines so bottom line doesn't get | |
// cut off. Also added padding-bottom so there's space btw the text and the divider. | |
.mat-mdc-list-item.mdc-list-item--with-leading-avatar.mdc-list-item--with-two-lines, | |
.mat-mdc-list-item.mdc-list-item--with-leading-checkbox.mdc-list-item--with-two-lines, | |
.mat-mdc-list-item.mdc-list-item--with-leading-icon.mdc-list-item--with-two-lines, | |
.mat-mdc-list-item.mdc-list-item--with-leading-avatar.mdc-list-item--with-three-lines, | |
.mat-mdc-list-item.mdc-list-item--with-leading-checkbox.mdc-list-item--with-three-lines, | |
.mat-mdc-list-item.mdc-list-item--with-leading-icon.mdc-list-item--with-three-lines { | |
height: auto; | |
padding-bottom: 8px; | |
} | |
.mat-mdc-list-item.mdc-list-item--with-three-lines, | |
.mat-mdc-list-item.mdc-list-item--with-two-lines { | |
height: auto; | |
padding-bottom: 13px; | |
.mat-mdc-list-item.mdc-list-item--with-three-lines, | |
.mat-mdc-list-item.mdc-list-item--with-two-lines { | |
height: auto; | |
padding-bottom: 13px; | |
&.mdc-list-item--with-leading-avatar, | |
&.mdc-list-item--with-leading-checkbox, | |
&.mdc-list-item--with-leading-icon { | |
padding-bottom: 8px; | |
} | |
... |
Fixes issue with Angular Components List component where the list item truncates on narrow screen widths (ie. mobile screens). Removes white-space wrap style and adds some height to primary lines for readability. Fixes b/291296866
Updates Angular Components List item component height if the list item has 3 lines. With the previous list item height of 88px the bottom lines letters like 'g' get cut off and less readable. Fixes b/247881646
Fixes Angular Components List item component with and without a leading avatar if it is has multiple lines for the height to auto adjust based on the content. Added padding-bottom for list items with a leading icon/avatar/checkbox to improve readability. Fixes b/291296866
Fixes bottom padding beneath Antular Components List item with leading avatar to provide a similar space as exists above the list item. Fixes b/291296866
Updates fix for Angular Components List component where the list item truncates on narrow screen widths (ie. mobile screens). Updates the padding to be more cohesive with previous styling. Fixes b/291296866
Updates the padding for the previous fix to Angular Components List component for the list item truncating on narrow screen widths. Fixes b/291296866
Updates the previous Angular Components List component fix to more closely align with previous screenshots and avoid additional screenshot test failures. Fixes b/291296866
b302644
to
369bb8c
Compare
Fixes issue with Angular Components List component where the list item truncates on narrow screen widths (ie. mobile screens). Removes white-space wrap style and adds some height to primary lines for readability.
Before screenshot
After screenshot
Fixes b/291296866