Skip to content

fix(web): nft image overlap on text in tx flow #5684

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

Closed
wants to merge 1 commit into from

Conversation

valens-carpentier
Copy link
Member

What it solves

This PR addresses the spacing issue between the NFT image and the text in the transaction flow.

Resolves ##3114

How this PR fixes it

Increased spacing value from 1 to 2 to ensure sufficient padding between the image and the text content.

How to test it

  1. Open the transaction flow where the NFT image appears.
  2. Inject the following test image using the console:
    <img style="display: block; margin: 0 auto;" height="64" width="64" alt="Test Image" src="https://picsum.photos/1920/1080">
    Inside the element: <div class="MuiBox-root mui-style-72fd9l"></div>

Screenshots

Screenshot 2025-04-11 at 13 57 07

Checklist

  • I've tested the branch on mobile (not applicable) 📱
  • I've documented how it affects the analytics (not applicable) 📊
  • I've written a unit/e2e test for it ((not applicable)) 🧑‍💻

Copy link

github-actions bot commented Apr 11, 2025

Copy link

📦 Next.js Bundle Analysis for @safe-global/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

Copy link

Coverage report for apps/web

St.
Category Percentage Covered / Total
🟡 Statements
76.34% (+0.09% 🔼)
14240/18654
🔴 Branches
54.91% (+0.08% 🔼)
3648/6644
🟡 Functions
60.46% (+0.22% 🔼)
2081/3442
🟡 Lines
77.96% (+0.05% 🔼)
12962/16627
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / constants.ts
72.94% (-0.55% 🔻)
97.06% (+0.18% 🔼)
100%
95.83% (+0.09% 🔼)
🔴
... / index.ts
14.71% (-0.22% 🔻)
0% 0%
15.87% (-0.26% 🔻)
🟢
... / logic.ts
98.04% (-1.96% 🔻)
80% (-20% 🔻)
100%
97.87% (-2.13% 🔻)
🟡
... / useGasLimit.ts
74.58%
60% (-2.5% 🔻)
71.43% 73.68%
🟢
... / utils.ts
90.32%
70% (-5% 🔻)
100% 89.29%

Test suite run success

1797 tests passing in 251 suites.

Report generated by 🧪jest coverage report action from 0fe070a

@@ -20,7 +20,7 @@ type SendNftBatchProps = {
}

const NftItem = ({ image, name, description }: { image: string; name: string; description?: string }) => (
<Stack direction="row" spacing={1} flexWrap="nowrap" alignItems="flex-start">
<Stack direction="row" spacing={2} flexWrap="nowrap" alignItems="flex-start">
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 not about a bigger spacing. 1 is OK too. It's about rectangular (non-square) images overflowing out of their container and overlapping with the text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, got it. That is why the changes are more complex than I expected. Would like to get the NFT from @liliya-soroka to make the correct changes. Thanks.

@katspaugh
Copy link
Member

@liliya-soroka could you share the Safe with that landscape NFT with @valens-carpentier?

@katspaugh
Copy link
Member

@liliya-soroka bump

@katspaugh
Copy link
Member

@valens-carpentier I'll have to close it for now...

@katspaugh katspaugh closed this May 27, 2025
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