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(card): titles trim #1124

Merged
merged 12 commits into from Nov 15, 2021
Merged

fix(card): titles trim #1124

merged 12 commits into from Nov 15, 2021

Conversation

rachelbt
Copy link
Contributor

@rachelbt rachelbt commented Nov 7, 2021

  • Added an option to set the numbers od lines before trim
  • Some visual QA

@github-actions
Copy link

github-actions bot commented Nov 7, 2021

🚀

Latest successful build of the PR deployed here.

🚀

Comment on lines 65 to 66
| `--title-line-number` | none | Controls the number of lines of the title that will be present before trim & ellipsis |
| `--subtitle-line-number` | none | Controls the number of lines of the sub-title that will be present before trim & ellipsis |
Copy link
Contributor

Choose a reason for hiding this comment

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

why wont we stick to the css property name here? line-clamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because than the same number will be for both title & subtitle

display: -webkit-box;
overflow: hidden;
-webkit-box-orient: vertical;
-webkit-line-clamp: var(--line-trim);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a way to do this with sass. I mean a scoped sass variable rather than another confusing css variable.
alternatively we could just set the -webkit-line-clamp twice for each selector

Copy link
Contributor Author

@rachelbt rachelbt Nov 8, 2021

Choose a reason for hiding this comment

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

I'm not sure understood what you meant... :(

@rachelbt rachelbt requested a review from yinonov November 9, 2021 07:51
<vwc-card label="All" heading="Card title" subtitle="Subtitle" header-icon="chat-line" support-text="Support Text">
<div style="height: 150px; width: 100%; background-color: red;" slot="media"></div>
<div slot="media"><img src="https://doodleipsum.com/300x150/flat?bg=7463D9&i=60361756b0ad15f4b3c4bd691f647ba9" alt="Sitting on Floor by Gustavo Pedrosa" /><p style="padding: 0 1.5rem; margin:0; text-align: center">Illustration by <a href="https://blush.design/artists/JycqpHYvuwwN3HzxBNyr/gustavo-pedrosa">Gustavo Pedrosa</a></p></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this one? It's here to test the use case in which media is just a colored div (it was requested by Joella in the past).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a duplication of the card with the color to create a card with an image

Comment on lines 65 to 66
| `--title-line-trim` | none | Controls the number of lines of the title that will be present before trim & ellipsis |
| `--subtitle-line-trim` | none | Controls the number of lines of the sub-title that will be present before trim & ellipsis |
Copy link
Contributor

Choose a reason for hiding this comment

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

why not line-clamp?

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 find trim more coherent, not all devs know the clam.

Copy link
Contributor

@yinonov yinonov Nov 10, 2021

Choose a reason for hiding this comment

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

but that's the actual css property

Copy link
Contributor

Choose a reason for hiding this comment

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

?

Comment on lines 65 to 66
| `--title-line-trim` | none | Controls the number of lines of the title that will be present before trim & ellipsis |
| `--subtitle-line-trim` | none | Controls the number of lines of the sub-title that will be present before trim & ellipsis |
Copy link
Contributor

Choose a reason for hiding this comment

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

?

@sonarcloud
Copy link

sonarcloud bot commented Nov 15, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rachelbt rachelbt merged commit f69d051 into master Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants