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 animation related to privacy warning message #1290

Closed
TalDerei opened this issue Jun 13, 2024 · 8 comments · Fixed by #1463
Closed

fix animation related to privacy warning message #1290

TalDerei opened this issue Jun 13, 2024 · 8 comments · Fixed by #1463
Assignees
Labels
good first issue Good for newcomers ui Related to user interface or ux design

Comments

@TalDerei
Copy link
Contributor

fix the height for the gas calculation, so only one jump happens

Screen.Recording.2024-06-12.at.11.47.22.PM.mov
@TalDerei TalDerei added good first issue Good for newcomers ui Related to user interface or ux design labels Jun 13, 2024
@github-project-automation github-project-automation bot moved this to 🗄️ Backlog in Penumbra web Jun 13, 2024
@grod220 grod220 moved this from 🗄️ Backlog to 📝 Todo in Penumbra web Jun 14, 2024
@grod220 grod220 added this to Labs web Jun 20, 2024
@grod220 grod220 moved this to 📝 Todo in Labs web Jun 20, 2024
@vacekj vacekj self-assigned this Jul 1, 2024
@vacekj vacekj moved this from 📝 Todo to 🏗 In progress in Labs web Jul 1, 2024
@vacekj vacekj moved this from 📝 Todo to 🏗 In progress in Penumbra web Jul 1, 2024
@vacekj
Copy link
Member

vacekj commented Jul 1, 2024

I see three simple ways to resolve this.
A) Set a fixed height on the Fee tier section - this looks odd to me with the empty space
B) Move the fee value next to the fee tier switch, and remove the gas icon, as we're not calling it gas anyway (at least for now).
C) Always display the value, but default to zero when we don't know the fee yet.

Image

Image

Image

I included screenshots for all cases. My personal preference is B, C, A in this order.

Curious to hear your thoughts @penumbra-zone/web-reviewers

@vacekj
Copy link
Member

vacekj commented Jul 1, 2024

after more info from Tal, the right way to proceed is to make both of the dialogs appear at the same time, instead of changing the height of the fee part directly.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jul 1, 2024

@VanishMax can you sanity-check the issue accurately captures the original intention for the suggestion you made?

@VanishMax
Copy link
Contributor

@vacekj thanks for the ideas with detailed screenshots. I believe the C options suits our needs – displaying the empty value for a fraction of a second is, in my opinion, better than having multiple jumps and worsening the cumulative layout shift (CLS). The option B looks goon on the screenshot but in many cases would not fit into the view

@vacekj
Copy link
Member

vacekj commented Jul 2, 2024

Going for option C here, debugging why Fee doesn't also contain the asssetId.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jul 2, 2024

debugging why Fee doesn't also contain the asssetId.

can you elaborate?

@vacekj
Copy link
Member

vacekj commented Jul 2, 2024

@TalDerei I'm going with option C, which displays a zero value before the actual fee is loaded, avoiding any CLS in the fee dialog. While implementing this, I realized that the fee param in GasFee (gas-fee.tsx:29) doesn't contain the assetId, just the amount. This leads to the fee display always defaulting to the stakingToken, in our case UM, and not the token that the fee is paid in (afaik after diggind deep into this with @grod220 ).

@vacekj vacekj moved this from 🏗 In progress to 🛑 Blocked in Labs web Jul 9, 2024
@vacekj vacekj moved this from 🛑 Blocked to 🏗 In progress in Labs web Jul 10, 2024
@vacekj vacekj linked a pull request Jul 10, 2024 that will close this issue
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Penumbra web Jul 11, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Labs web Jul 11, 2024
@TalDerei
Copy link
Contributor Author

thanks for taking this on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers ui Related to user interface or ux design
Projects
Archived in project
3 participants