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 dynamically resizing label with highlight, for adaptively filling up horizontal space #1397

Merged
merged 8 commits into from
May 10, 2024

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Apr 30, 2024

This PR adds:

  • Level 1: cutAroundMatch(): utility function for getting a fixed-length a part of a text string which contains the match to the pattern (if any)
  • Level 2: HighlightedTrimmedText: component that takes a text and a search pattern, and displays the text, with the matching part (if any) highlighted, the whole thing trimmed to a specific length around the highlight. (Or the beginning if there is no match.) This uses cutAroundMatch().
  • Utility: AdaptiveDynamicTrimmer: a component which can display content, dynamically filling up the available space until scrolling/overflow happens. The dynamic part in name references to the fact that it expects a function prop to provide a version of the content shortened for any specific length. This function will be called with various length, to find the best version of the content that fits. Note that this function can do smart things to get the best representation for the given length, which might be more complex than simply getting the beginning, the end or the middle.
  • Level 3: AdaptiveHighlightedText: a component that displays text with a part highlighted, adaptively trimmed around the highlight to fit the maximum available length without overflow. This uses HighlightedTrimmedText and AdaptiveDynamicTrimmer.

Changes:

  • Teach TrimEndLinkLabel component to support highlighting parts within the labels, and show the relevant part instead of always showing the beginning of the label.

Testing

None of these changes are immediately visible on the UI. To see this in action, check out some other PRs that use this:

Copy link

github-actions bot commented Apr 30, 2024

Deployed to Cloudflare Pages

Latest commit: a6f8cc8e475b633b3287f6b5dde345e7e58eff9b
Status:✅ Deploy successful!
Preview URL: https://7895d3cf.oasis-explorer.pages.dev

@csillag
Copy link
Contributor Author

csillag commented Apr 30, 2024

Code extracted from the Pontus-X branch

@csillag csillag force-pushed the csillag/adaptive-text-shortener-and-highlighter branch 2 times, most recently from b36a4a4 to 6cb1ec6 Compare May 7, 2024 13:23
@csillag csillag marked this pull request as ready for review May 7, 2024 13:26
@csillag csillag requested review from lukaw3d, buberdds and lubej as code owners May 7, 2024 13:26
@csillag csillag mentioned this pull request May 7, 2024
@csillag csillag force-pushed the csillag/adaptive-text-shortener-and-highlighter branch 3 times, most recently from d88be0c to 3b22b90 Compare May 9, 2024 09:53
src/app/components/HighlightedText/text-cutting.ts Outdated Show resolved Hide resolved
src/app/components/HighlightedText/text-cutting.ts Outdated Show resolved Hide resolved
src/app/components/HighlightedText/text-cutting.ts Outdated Show resolved Hide resolved
src/app/components/HighlightedText/text-cutting.ts Outdated Show resolved Hide resolved
Comment on lines +56 to +60
extraTooltip={
extraTooltip ? (
<>
<InfoIcon />
{extraTooltip}
Copy link
Member

Choose a reason for hiding this comment

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

It's confusing that AdaptiveHighlightedText's extraTooltip prepends the icon and AdaptiveDynamicTrimmer's extraTooltip doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Will think about this later.

@lukaw3d
Copy link
Member

lukaw3d commented May 10, 2024

example (check with 630px width)

ec1a5160 oasis-explorer pages dev_mainnet_consensus_proposal_3_voter=ell (1)
@donouwens is this the expected behavior

Copy link
Member

@lukaw3d lukaw3d left a comment

Choose a reason for hiding this comment

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

those things can be fixed as separate PR, if it adds too many conflicts

@csillag csillag force-pushed the csillag/adaptive-text-shortener-and-highlighter branch 3 times, most recently from e67837a to 46f6996 Compare May 10, 2024 12:48
@csillag csillag force-pushed the csillag/adaptive-text-shortener-and-highlighter branch from 46f6996 to 756b4a1 Compare May 10, 2024 13:01
@csillag
Copy link
Contributor Author

csillag commented May 10, 2024

example (check with 630px width)

ec1a5160 oasis-explorer pages dev_mainnet_consensus_proposal_3_voter=ell (1)

@donouwens is this the expected behavior

You mean the fact that the highlight has some margin, so we are selecting half a character to the left and to the right?
If that's what you mean, then yes, I think this is what we wanted. This is also why we made the yellow highlight class slightly transparent, so that it's not an issue that we partially cover other characters. (But @donouwens can confirm/correct.)

@csillag csillag force-pushed the csillag/adaptive-text-shortener-and-highlighter branch from 8561aed to e3c7af2 Compare May 10, 2024 14:30
@csillag csillag enabled auto-merge May 10, 2024 14:32
When we trim a text around a pattern match, and later we try
to highlight the match, pass the information along
from the trimming phase to the highlighting phase.

This avoids a round of unnecessary text matching, and fixes the corner case
when the trimmed part is actually shorter than the pattern.
@csillag csillag force-pushed the csillag/adaptive-text-shortener-and-highlighter branch from e3c7af2 to a6f8cc8 Compare May 10, 2024 14:33
@csillag csillag merged commit 454f3eb into master May 10, 2024
7 checks passed
@csillag csillag deleted the csillag/adaptive-text-shortener-and-highlighter branch May 10, 2024 14:35
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.

2 participants