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

fix: handle long nft ids in simulation amount pill #25252

Merged

Conversation

OGPoyraz
Copy link
Member

@OGPoyraz OGPoyraz commented Jun 12, 2024

Description

This PR aims to fix styling AmountPill component used in the SimulationDetails. When there is a long NFT id given it overflows and shrinks the AssetPill. If there is a long id given, now we want to shorten it.

Also the fixes the tooltip text which is currently overflowing the container.

Open in GitHub Codespaces

Related issues

Fixes: #24178

Manual testing steps

  1. Any transaction with long NFT id. (Any https://app.ens.domains/ will trigger a long NFT Id)

Screenshots/Recordings

Before

image

After

Screenshot 2024-06-12 at 11 56 38

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.

@OGPoyraz OGPoyraz added the team-confirmations Push issues to confirmations team label Jun 12, 2024
@OGPoyraz OGPoyraz requested review from a team as code owners June 12, 2024 10:22
@OGPoyraz OGPoyraz linked an issue Jun 12, 2024 that may be closed by this pull request
@@ -78,6 +87,7 @@ export const AmountPill: React.FC<{
position="bottom"
title={tooltipParts.join(' ')}
wrapperStyle={{ minWidth: 0 }}
theme="word-break-all"
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately Tooltip ignores style property hence this is the only proper solution that I find.

@OGPoyraz OGPoyraz force-pushed the 24178-bug-not-ellipsing-long-nft-id-within-simulations branch from 9e9d403 to 0f2adf9 Compare June 12, 2024 10:35
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 64.87%. Comparing base (038f3d6) to head (f9fd048).
Report is 2 commits behind head on develop.

Files Patch % Lines
ui/helpers/utils/util.js 42.86% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25252      +/-   ##
===========================================
- Coverage    64.89%   64.87%   -0.02%     
===========================================
  Files         1382     1382              
  Lines        54770    54795      +25     
  Branches     14372    14381       +9     
===========================================
+ Hits         35539    35547       +8     
- Misses       19231    19248      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [bb43054]
Page Load Metrics (135 ± 185 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6211481126
domContentLoaded9121011
load401815135385185
domInteractive9121011
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 199 Bytes (0.00%)
  • common: 12 Bytes (0.00%)

ui/helpers/utils/util.js Show resolved Hide resolved
ui/helpers/utils/util.js Outdated Show resolved Hide resolved
ui/helpers/utils/util.js Outdated Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [f9fd048]
Page Load Metrics (55 ± 7 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6912086147
domContentLoaded9201121
load4210355157
domInteractive9201121
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 245 Bytes (0.00%)
  • common: 317 Bytes (0.00%)

Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

🚀

@OGPoyraz OGPoyraz merged commit 7e6fb5d into develop Jun 20, 2024
74 checks passed
@OGPoyraz OGPoyraz deleted the 24178-bug-not-ellipsing-long-nft-id-within-simulations branch June 20, 2024 16:50
@github-actions github-actions bot locked and limited conversation to collaborators Jun 20, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Not ellipsing long NFT ID within simulations
4 participants