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

Fixing activity title text truncation #10601

Merged
merged 1 commit into from Mar 8, 2021
Merged

Fixing activity title text truncation #10601

merged 1 commit into from Mar 8, 2021

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Mar 8, 2021

Fixes #9997

Screen Shot 2021-03-07 at 11 31 21 PM

Manual testing steps:

  • Access an account with activity
  • Via another transaction (or the inspector tool), create a long activity title
  • Ensure that the title text truncates appropriately
  • Edit balance amount title so it truncates
  • Ensure that there is no overlap when both titles are truncated

@ryanml ryanml requested a review from a team as a code owner March 8, 2021 06:48
@ryanml ryanml requested a review from Gudahtt March 8, 2021 06:48
@@ -27,9 +27,7 @@ export default function ListItem({
{React.isValidElement(title) ? (
title
) : (
<button onClick={onClick}>
Copy link
Contributor Author

@ryanml ryanml Mar 8, 2021

Choose a reason for hiding this comment

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

There were existing rules for list-item__title regarding text truncation, but being the direct child of a button tag was blocking them from taking effect. This button element and associated onClick value are actually redundant, as the list item wrapper handles the onClick event for all of its contents:

Copy link
Member

Choose a reason for hiding this comment

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

Huh, interesting find!

The use of button here was important though - it allowed keyboard navigation. Though... now that I'm testing it... it seems to have made it focusable, but pressing "Enter" did nothing 😞.

Well, given that this already wasn't keyboard-friendly, this doesn't need to block here. But that's something we'll want to do. I'd suggest changing the div with the onClick set to a button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestion makes sense here 👍 Updated the outer container to be of type button:

Screen Shot 2021-03-08 at 10 26 53 AM

@ryanml ryanml self-assigned this Mar 8, 2021
@metamaskbot
Copy link
Collaborator

Builds ready [187c782]
Page Load Metrics (794 ± 170 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45282905526
domContentLoaded3861549792353170
load3881552794354170
domInteractive3861549792353170

Gudahtt
Gudahtt previously approved these changes Mar 8, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

As noted in a comment, this does impact accessibility, but this list already had issues there.

@darkwing
Copy link
Contributor

darkwing commented Mar 8, 2021

Awesome work @ryanml !

@metamaskbot
Copy link
Collaborator

Builds ready [a2b419e]
Page Load Metrics (621 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint458062105
domContentLoaded3817326208038
load3837336218038
domInteractive3817326208038

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [73ea889]
Page Load Metrics (584 ± 27 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43695484
domContentLoaded3816725825527
load3826735845527
domInteractive3806725825527

@ryanml ryanml merged commit d68466f into develop Mar 8, 2021
@ryanml ryanml deleted the fix-9997 branch March 8, 2021 18:12
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activity title overlaps amount if too long
4 participants