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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding new .Truncate class and deprecating css-truncate #1358

Merged
merged 12 commits into from May 20, 2021
Merged

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Apr 23, 2021

This PR re-writes css-truncate https://primer.style/view-components/components/truncate into a flexbox driven solution.

Doing so gives us these advantages over the previous solution:

  • 馃毇 max-width is no longer needed to truncate and we don't need the magic 135px hardcoded.
  • 馃憤馃徎 .Truncate can do a reverse truncate where the ... shows up at the beginning instead of the end.
  • 馃憤馃徎 .Truncate can have multiple items with a "primary" item that doesn't truncate at the same speed as the rest.

Work based on work done by @vdepizzol https://codepen.io/vdepizzol/pen/VwPVyZa?editors=1100 In his example he used grid which is a nice implementation but there isn't enough support of the grid-template-columns property to make this work.

/cc @primer/ds-core

@changeset-bot
Copy link

changeset-bot bot commented Apr 23, 2021

馃 Changeset detected

Latest commit: e4ee5f2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@simurai
Copy link
Contributor

simurai commented Apr 26, 2021

Currently it's possible to truncate a single element with css-truncate css-truncate-overflow without having to nest an extra <span>. Maybe we could allow using a combo like Truncate Truncate-text?

<span class="Truncate Truncate-text">Truncate some text</span>

Or just use Truncate for single use cases and then also add Truncate Truncate--multiple

<span class="Truncate">Truncate some text</span>

<span class="Truncate Truncate--multiple">
  <span class="Truncate-item">Truncate item 1</span>
  <span class="Truncate-item">Truncate item 2</span>
  <span class="Truncate-item">Truncate item 3</span>
</span>

there isn't enough support of the grid-template-columns property

We might need to do some testing but it should be fine as long as we don't use subgrid? 馃

.Truncate can have multiple items with a "primary" item that doesn't truncate at the same speed as the rest.

This doesn't seem to work and .Truncate-text--primary never truncates and gets cut off when not enough space.

truncate

Not sure if it possible to also truncate .Truncate-text--primary but only once the non-primary text truncated first. Might not be possible with flexbox alone?

Here a Codepen from trying to fix the file header https://github.com/github/github/issues/117827#issuecomment-514511496. Also relies on grid-template-columns. It keeps short file names un-truncated until there is almost no space. But starts to truncate the file name earlier, if the file name is long.

truncate

@vdepizzol
Copy link
Contributor

there isn't enough support of the grid-template-columns property

We might need to do some testing but it should be fine as long as we don't use subgrid? 馃

The compatibility issue stylelint has warned for grid-template-columns seems incorrect. We discussed this last Friday here.

@jonrohan
Copy link
Member Author

I made some updates: preview

Found it: github/github#119636. Not sure if it's still an issue in Safari. The other problem was with dotfiles where the dot moved to the end:

ooo, that's a bad bug, I'm removing the --beginning option.

This doesn't seem to work and .Truncate-text--primary never truncates and gets cut off when not enough space.

This is fixed using flex-basis see latest preview

Issues with grid support aside I'm not convinced it's a better alternative to flexbox here. I think flexbox gives us some more options about what parts of a string we truncate. see second example

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

This is fixed using flex-basis

These multi .Truncate-text--primary are pretty neat. 馃槏

truncate

Something I was wondering in Slack was if we should allow simple cases like <span class="Truncate Truncate-text">some text</span>. Without the need for an extra nested element just to truncate some text. But as long as we only use the Truncate ViewComponent it might be fine as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants