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.

Sign up for GitHub

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

Jump to bottom

feat: Better handle very long names in the name component #28560

Merged
merged 2 commits into from
Nov 21, 2024
Merged

feat: Better handle very long names in the name component #28560

merged 2 commits into from
Nov 21, 2024

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Nov 20, 2024 •
edited by github-actions bot
Loading

Description

Truncates long names (>15 characters).

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3630

Manual testing steps

  1. Trigger a new confirmation
  2. Add a long petname, by clicking an address and writing it in the input field
  3. The name should be truncated with an ellipsis.

Screenshots/Recordings

Before

After

Screenshot 2024-11-20 at 11 24 21

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@pedronfigueiredo pedronfigueiredo added the team-confirmations Push issues to confirmations team label Nov 20, 2024
@pedronfigueiredo pedronfigueiredo self-assigned this Nov 20, 2024
@pedronfigueiredo pedronfigueiredo requested a review from a team as a code owner November 20, 2024 11:24
@github-actions GitHub Actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

matthewwalsh0
matthewwalsh0 requested changes Nov 20, 2024
View reviewed changes
ui/components/app/name/name.tsx Outdated
@@ -60,6 +60,20 @@ function formatValue(value: string, type: NameType): string {
}
}

export function formatName(name: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

shortenString from ui/helpers/utils/util.js?

@metamaskbot
Copy link
Collaborator

Builds ready [b50e87b]
Page Load Metrics (2353 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33526742149626301
domContentLoaded18212605230618790
load18832665235319091
domInteractive1796532010
backgroundConnect9140433818
firstReactRender813101437636
getState552221294220
initialActions01000
loadScripts13011942170915775
setupStore882272914
uiStartup214334232854285137
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 125 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

jpuri
jpuri previously approved these changes Nov 20, 2024
View reviewed changes
jpuri
jpuri reviewed Nov 20, 2024
View reviewed changes
ui/components/app/name/name.tsx
@@ -135,7 +141,7 @@ const Name = memo(
)}
{hasDisplayName ? (
<Text className="name__name" variant={TextVariant.bodyMd}>
{name}
{formattedName}
</Text>
) : (
<Text className="name__value" variant={TextVariant.bodyMd}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will be nice to have some unit test coverage also

@metamaskbot
Copy link
Collaborator

Builds ready [a5b411b]
Page Load Metrics (2252 ± 128 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27329392157502241
domContentLoaded188328782194239115
load193729592252267128
domInteractive33265756431
backgroundConnect10223605627
firstReactRender563031428240
getState492201225225
initialActions01000
loadScripts13212249163720297
setupStore782303115
uiStartup221333852714403194
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 125 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@pedronfigueiredo pedronfigueiredo added this pull request to the merge queue Nov 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024
@pedronfigueiredo pedronfigueiredo added this pull request to the merge queue Nov 21, 2024
Merged via the queue into develop with commit f455a6e Nov 21, 2024
75 checks passed
@pedronfigueiredo pedronfigueiredo deleted the pnf/3630 branch November 21, 2024 12:37
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@matthewwalsh0 matthewwalsh0 matthewwalsh0 approved these changes

@jpuri jpuri jpuri approved these changes

Assignees

@pedronfigueiredo pedronfigueiredo

Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-confirmations Push issues to confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pedronfigueiredo @metamaskbot @jpuri @matthewwalsh0