Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-hyperlink] Hyperlink ellipses #4072

Merged
merged 10 commits into from
Mar 25, 2024
Merged

[terra-hyperlink] Hyperlink ellipses #4072

merged 10 commits into from
Mar 25, 2024

Conversation

kenk2
Copy link
Contributor

@kenk2 kenk2 commented Mar 21, 2024

Summary

What was changed:
A hyperlink will now truncate with an ellipses if its content should overflow.

Why it was changed:
Feature request from a consuming team

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-10303


Thank you for contributing to Terra.
@cerner/terra

@kenk2 kenk2 marked this pull request as ready for review March 22, 2024 14:52
@kenk2 kenk2 requested a review from a team as a code owner March 22, 2024 14:52
border-style: dashed;
margin-bottom: 10px;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is only used for a single file, can we use inline styles for this? e.g.
https://css-tricks.com/different-ways-to-write-css-in-react/#aa-write-inline-styles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

outline: none;
text-decoration: var(--terra-hyperlink-text-decoration, underline);
touch-action: manipulation; // Enable fast tap interaction in webkit browsers; see https://webkit.org/blog/5610/more-responsive-tapping-on-ios/
vertical-align: baseline;
display: inline-flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing the style from inline-block to inline-flex? I would not expect this change to be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're needing to have 2 spans with 1 being an ellipses but another being on the same level, we'll need to go with this.

Copy link
Contributor

@BenBoersma BenBoersma Mar 22, 2024

Choose a reason for hiding this comment

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

Also inline-block wasn't allowing us to properly truncate without the hyperlink "clickable" area extending past the "visual" area of the hyperlink or other visual issues. Using inline-flex, we were able to properly have the "text" portion of hyperlink resize itself so it would truncate without issue while allowing the icon to always render at full size.

@@ -245,7 +245,9 @@ class Hyperlink extends React.Component {
ref={this.linkRef}
>
<span className={cx('button-inner')}>
{text || children}
<span className={cx('inner-text')}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be able to accomplish this with one span?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the requirements, we need to make only the text have an ellipsis while the icon stays at the same level. So we need to use 2 spans for this. Otherwise it will start wrapping around if we try to implement the ellipses without having an additional inner span.

@kenk2 kenk2 requested a review from sdadn March 22, 2024 15:59
@kenk2 kenk2 requested a review from cm9361 March 22, 2024 20:25
@github-actions github-actions bot temporarily deployed to preview-pr-4072 March 22, 2024 20:27 Destroyed
Copy link
Contributor

@adoroshk adoroshk left a comment

Choose a reason for hiding this comment

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

LGTMM, works as expected.

@kenk2 kenk2 merged commit 788f90c into main Mar 25, 2024
22 checks passed
@kenk2 kenk2 deleted the hyperlink-ellipses-trunc branch March 25, 2024 17:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants