chore(Truncate): added modifier and example for max characters logic#7439
chore(Truncate): added modifier and example for max characters logic#7439mcoker merged 5 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-pr-7439.surge.sh A11y report: https://patternfly-pr-7439-a11y.surge.sh |
There was a problem hiding this comment.
LGTM. Left a couple of nit comments. Also just to make sure, it looks like the ellipsis that is displayed via text-truncate: ellipsis is different from ... characters in chrome/ff/safari. Looks like the ellipsis match the … character. I included those and a variation using … here if you want to look in any other browsers https://codepen.io/mcoker/pen/GgRLaWZ
An additional thought - now that adding a class can be seen as a breaking change, I wonder if we should add classes for the three elements in the new examples. Or at least the visible text and ellipsis. They seem like structural elements of this variation of the component. Maybe something like .[truncate]__text for the visible text and .[truncate]__clip or __overflow?
| | `.pf-v6-c-truncate__start` | `<span>` | Defines the truncate component starting text. | | ||
| | `.pf-v6-c-truncate__end` | `<span>` | Defines the truncate component ending text. | | ||
| | `.pf-m-ignore-resizing` | `span.pf-v6-c-truncate` | Modifies the truncate component to ignore resizing and container widths for truncation. | | ||
| | `.pf-v6-screen-reader` | `.pf-v6-c-truncate span` | Visually hides text when truncation is meant to occur based on a maximum amount of characters rather than resizing/container width. | |
There was a problem hiding this comment.
This docs table typically only lists classes for the component. We would have traditionally put a line like this in the a11y docs table.
There was a problem hiding this comment.
Just to confirm, is this in regards to the screen reader class row, or both the added rows?
There was a problem hiding this comment.
Sorry about that - just the screen reader class row. Though on .pf-m-ignore-resizing, is there a reason you added span.pf-v6-c-truncate and not just .pf-v6-c-truncate?
And I'm not wild about .pf-m-ignore-resizing, but I'm not wild about naming things either, so I didn't mention it 😂 WDYT about .pf-m-static or .pf-m-fixed (as in fixed width/characters) instead?
There was a problem hiding this comment.
- I may have been thinking of the screen reader one in reverse 😆 no real reason to have it written that way, though, you're right
- Both of those sound much better (though the next hill is to decide on which one). Do you think we need to worry about the lack of context behind them at all? Like
pf-m-fixedmay invoke the thought of "fixed dimensions" or 'fixed position', but the context is like you said "fixed characters" more than anything. The docs would of course relay the context, was just curious.
There was a problem hiding this comment.
@thatblindgeye good question, we traditionally use .pf-m-static for "static" positioning (static vs dynamic/sticky). We use .pf-m-static-width in <Progress> https://www.patternfly.org/components/progress/html#outside-static-width-measure. That or .pf-m-static-chars seem fine, but I'm also good with .pf-m-static here since <Truncate> doesn't really have layout for positioning like components where we're currently using .pf-m-static, so I don't think we'd ever run into a conflict there.
|
|
||
| ### Based on max characters | ||
|
|
||
| Apply the `pf-m-ignore-resizing` class to the `pf-v6-c-truncate` element to implement truncation based on an amount of characters rather than a parent container width. You must ensure that the omission designator (typically an ellipsis) is removed from the accessibility tree by wrapping it in a `<span>` with the `aria-hidden="true"` attribute. |
There was a problem hiding this comment.
Nit - just adding . to be consistent with the rest of the docs.
| Apply the `pf-m-ignore-resizing` class to the `pf-v6-c-truncate` element to implement truncation based on an amount of characters rather than a parent container width. You must ensure that the omission designator (typically an ellipsis) is removed from the accessibility tree by wrapping it in a `<span>` with the `aria-hidden="true"` attribute. | |
| Apply the `.pf-m-ignore-resizing` class to the `.pf-v6-c-truncate` element to implement truncation based on an amount of characters rather than a parent container width. You must ensure that the omission designator (typically an ellipsis) is removed from the accessibility tree by wrapping it in a `<span>` with the `aria-hidden="true"` attribute. |
|
|
||
| ### Notes | ||
| The truncate component contains two child elements, `.pf-v6-c-truncate__start` and `.pf-v6-c-truncate__end`. If both `start` and `end` are present within `.pf-v6-c-truncate`, trucation will occur in the middle of the string. If only `.pf-v6-c-truncate__start` is present, truncation will occur at the end of the string. If only `.pf-v6-c-truncate__end` is present, truncation will occur at the beginning of the string. A `.pf-v6-c-popover` will be automatically applied to the PatternFly React implementation. `‎` must be included at the end of string to denote the ending punctuation mark. Otherwise it will occur and the beggining of truncation for a `pf-v6-c-truncate__end` element. | ||
| The default behavior of the `Truncate` component is to truncate based on whether the content can fit within the width of its parent container, and to prevent text from wrapping. The following examples that use this default behavior render the `Truncate` component inside a resizable container, allowing you to see how the parent container width affects the truncation. |
There was a problem hiding this comment.
I could be mistaken, but I think if we're referring to a component name, it's lower-case and a regular word. Do you know of a different standard?
| The default behavior of the `Truncate` component is to truncate based on whether the content can fit within the width of its parent container, and to prevent text from wrapping. The following examples that use this default behavior render the `Truncate` component inside a resizable container, allowing you to see how the parent container width affects the truncation. | |
| The default behavior of the truncate component is to truncate based on whether the content can fit within the width of its parent container, and to prevent text from wrapping. The following examples that use this default behavior render the truncate component inside a resizable container, allowing you to see how the parent container width affects the truncation. |
There was a problem hiding this comment.
I think it's what I'm used to when it comes to React example descriptions. cc @edonehoo thoughts/opinions?
There was a problem hiding this comment.
So if you're referring to the general pf component (like as a concept) then it's lowercase, like "truncate". But if you're referring to the "React" component/subcomponent, then I usually go for "<Truncate>". Kind of a small distinction. I tend to reserve the <Component> format only when it's important to clarify, like when instructing users to pass a prop to a specific component.
I haven't written much for Core docs, so that's an interesting question. Based off of what I just wrote here ^, and looking at the context on this page, it seems like we would use the lowercase/non-code-formatted "truncate". But would be curious to hear your opinions too
There was a problem hiding this comment.
Thanks @edonehoo! We used to title case component names at some point, but it was inconsistent, and not for any particular reason. It was just how some folks chose to write it. A while back content did an audit and created some standards, then we did a sweep through the content and updated everything to be lower case, so I'd say go with lower case "truncate" like in my comment above 👍
Yeah that would make sense to me. Do you think we should use a new class for the visiblle text in this new implementation? Sorta ties into 1 of my questions it the original comment above re: reusing the |
abb80c3 to
0ef195a
Compare
| | `.pf-v6-c-truncate__start` | `.pf-v6-c-truncate span` | Defines the truncate component starting text. Only to be used when the `.pf-m-fixed` class is **not** applied to the `.pf-v6-c-truncate` element. | | ||
| | `.pf-v6-c-truncate__end` | `.pf-v6-c-truncate span` | Defines the truncate component ending text. Only to be used when the `.pf-m-fixed` class is **not** applied to the `.pf-v6-c-truncate` element. | | ||
| | `.pf-v6-c-truncate__text` | `.pf-v6-c-truncate.pf-m-fixed span` | Defines the visible truncate component text when the `pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | |
There was a problem hiding this comment.
Nits - I don't mind the way you wrote it originally, but it isn't consistent. We use an element (eg <span>) and/or classname for the "applied" column.
You could also update the description for __start (and the others) to say "Required when using default/end or middle truncation, except when .pf-m-fixed is applied to the .pf-v6-c-truncate element." I don't really care either way, but wanted to mention it if you like it.
| | `.pf-v6-c-truncate__start` | `.pf-v6-c-truncate span` | Defines the truncate component starting text. Only to be used when the `.pf-m-fixed` class is **not** applied to the `.pf-v6-c-truncate` element. | | |
| | `.pf-v6-c-truncate__end` | `.pf-v6-c-truncate span` | Defines the truncate component ending text. Only to be used when the `.pf-m-fixed` class is **not** applied to the `.pf-v6-c-truncate` element. | | |
| | `.pf-v6-c-truncate__text` | `.pf-v6-c-truncate.pf-m-fixed span` | Defines the visible truncate component text when the `pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | | |
| | `.pf-v6-c-truncate__start` | `<span>` | Defines the truncate component starting text. Only to be used when the `.pf-m-fixed` class is **not** applied to the `.pf-v6-c-truncate` element. | | |
| | `.pf-v6-c-truncate__end` | `<span>` | Defines the truncate component ending text. Only to be used when the `.pf-m-fixed` class is **not** applied to the `.pf-v6-c-truncate` element. | | |
| | `.pf-v6-c-truncate__text` | `<span>` | Defines the visible truncate component text when the `.pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | |
| {{#if truncate-start--attribute}} | ||
| {{{truncate-start--attribute}}} | ||
| {{/if}} | ||
| aria-hidden="true"> |
There was a problem hiding this comment.
nit - we'll add the --attribute bit last to make sure it overrides any previously set attributes.
| {{#if truncate-start--attribute}} | |
| {{{truncate-start--attribute}}} | |
| {{/if}} | |
| aria-hidden="true"> | |
| aria-hidden="true" | |
| {{#if truncate-start--attribute}} | |
| {{{truncate-start--attribute}}} | |
| {{/if}}> |
|
|
||
| &.pf-m-fixed { | ||
| display: inline; | ||
| min-width: 0; |
There was a problem hiding this comment.
Just a nice to have, you could add align-items: revert; (and use revert for min-width, too). Some flex/grid/align- properties now work on non-flex/grid things, so this is an extra line of code, but might help us avoid a bug in the future. Also min-width: revert will let min-width be whatever it needs to be, as determined by the browser. If .pf-v6-c-truncate.pf-m-fixed is a child of a flex/grid layout, for example, min-width: 0 would have a very specific, possibly unwanted effect.
|
|
||
| ### Based on max characters | ||
|
|
||
| Apply the `.pf-m-fixed` class to the `.pf-v6-c-truncate` element to implement truncation based on a fixed amount of characters rather than a parent container width. You must ensure that the omission designator (typically an ellipsis) is removed from the accessibility tree by wrapping it in a `<span>` with the `aria-hidden="true"` attribute. |
There was a problem hiding this comment.
Do you still need the span/aria-hidden blurb now that it comes from the component? I'd say not, but if you want to keep it, maybe just make it as a recommendation to ensure the element has aria-hidden="true"?
| Apply the `.pf-m-fixed` class to the `.pf-v6-c-truncate` element to implement truncation based on a fixed amount of characters rather than a parent container width. You must ensure that the omission designator (typically an ellipsis) is removed from the accessibility tree by wrapping it in a `<span>` with the `aria-hidden="true"` attribute. | |
| Apply the `.pf-m-fixed` class to the `.pf-v6-c-truncate` element to implement truncation based on a fixed amount of characters rather than a parent container width. |
| | `.pf-v6-c-truncate` | `<span>` | Initiates the truncate component. **Required** | | ||
| | `.pf-v6-c-truncate__start` | `<span>` | Defines the truncate component starting text. **Required** when using default/end or middle truncation, **except** for when the `.pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | | ||
| | `.pf-v6-c-truncate__end` | `<span>` | Defines the truncate component ending text. **Required** when using start or middle truncation, **except** for when the `.pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | | ||
| | `.pf-v6-c-truncate__text` | `<span>` | Defines the visible truncate component text. **Required** and should only be used when the `pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | |
There was a problem hiding this comment.
| | `.pf-v6-c-truncate__text` | `<span>` | Defines the visible truncate component text. **Required** and should only be used when the `pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | | |
| | `.pf-v6-c-truncate__text` | `<span>` | Defines the visible truncate component text. **Required** and should only be used when the `.pf-m-fixed` class is applied to the `.pf-v6-c-truncate` element. | |
There was a problem hiding this comment.
LPTM! Left a wee lil' suggestion but it's fine as-is.
|
I have a question around the behavior. In the case of truncating after a max number of characters, should it also continue to truncate when it's parent container continues to shrink and not trigger a horizontal scroll. Why would we not want it to behave like that? |
|
@sg00dwin I think in that case we would want to recommend that consumers use the default implementation that does truncate based on container width. This is sort of a specific use-case where the consumer should know that the container will always be large enough for the truncated content. Though I guess another question might be would we want to have the text wrap if it gets too long for its container? It really stems from the ask in React and trying to get a "true middle" truncation when the font-size and container width resizing would constantly change what the "true middle" is, and all of that making it pretty difficult to determine what the "true middle" should be at any given time. The decision in React was to instead implement this sort of behavior where truncation is soley based on a max number of characters instead of resizing/container width. |
|
@thatblindgeye Ah ok that's understandable. |
|
🎉 This PR is included in version 6.3.0-prerelease.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #7438
Couple of things:
__startand__endelements for this new implementation and update additional styles base don this new modifier class?