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

Add .css-truncate-overflow #978

Merged
merged 4 commits into from
Nov 8, 2019
Merged

Add .css-truncate-overflow #978

merged 4 commits into from
Nov 8, 2019

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Nov 7, 2019

This adds a .css-truncate-overflow modifier that truncates any overflowing text regardless of the element's width.

truncate-overflow

📖 Docs preview

Motivation

Currently .css-truncate-target is meant for "inline" elements. It has display: inline-block; and max-width: 125px;. To truncate "block" elements, a .d-block class (or similar) with an inline style of max-width: none; is needed.

Having just a single class (.css-truncate-overflow) should make it easier and less confusing.

Before

<div class="css-truncate css-truncate-target d-block" style="max-width: none">

After

<div class="css-truncate css-truncate-overflow">

TODO

  • Add styles
  • Update documentation

TODO on dotcom

Nothing is necessary. But we could clean up some of the overrides.


Closes #970

/cc @primer/ds-core

@vercel
Copy link

vercel bot commented Nov 7, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/731j6osn2
🌍 Preview: https://primer-css-git-truncate.primer.now.sh

// css-truncate will shorten text with an ellipsis. The maximum width
// of the truncated text can be changed by overriding the max-width
// of the .css-truncate-target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff is a bit messy with these "double selectors". 😩 Without it's:

// Both share these properties
.css-truncate-overflow,
.css-truncate-target {
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
}

// .css-truncate-target has these additional properties
.css-truncate-target {
  display: inline-block;
  max-width: 125px;
  vertical-align: top;
}

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 also tempting to rename it to something like:

// Default
.Truncate {
  overflow: hidden;
  text-overflow: ellipsis;
  white-space: nowrap;
}

// Modifier for inline elements
.Truncate--inline {
  display: inline-block;
  max-width: 125px;
  vertical-align: top;
}

Would require some bigger refactor on dotcom.

@shawnbot shawnbot mentioned this pull request Nov 7, 2019
19 tasks
Copy link
Contributor

@shawnbot shawnbot 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 great. I made some copy suggestions to (IMO) make the whole thing read better (e.g. "Use the css-truncate class to x" instead of ".css-truncate does x", which refers to the selector and not the thing you actually write), but feel free to adjust them as you see fit before merging. 🚀

Co-Authored-By: Shawn Allen <shawn.allen@github.com>
Co-Authored-By: Shawn Allen <shawn.allen@github.com>
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.

2 participants