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

chore: refactor header for tokens #11855

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

brianacnguyen
Copy link
Contributor

Description

This PR updates the Header for the Token details view, based on this design update by @amandaye0h . Specifically

  • Left and right button icons are now using the ButtonIcon component with the large size
  • Overflow icon is updated to vertical version
  • Alignment of header is now top aligned

Related issues

Fixes: #11854

Manual testing steps

  1. Go to Wallet
  2. Click on a token

Screenshots/Recordings

Before

Token 2 - Dark
Token 2 - Light

After

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-17.at.15.48.18.mp4

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.

@brianacnguyen brianacnguyen added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-design-system All issues relating to design system in Mobile labels Oct 17, 2024
@brianacnguyen brianacnguyen self-assigned this Oct 17, 2024
@brianacnguyen brianacnguyen requested review from a team as code owners October 17, 2024 22:57
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.

@brianacnguyen brianacnguyen added the Run Smoke E2E Triggers smoke e2e on Bitrise label Oct 17, 2024
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 51ff159
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d2622345-500a-4fa4-8aa4-2f4db063b5db

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Nice work 🚀 LGTM!

@brianacnguyen brianacnguyen added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 17, 2024
Copy link
Contributor

github-actions bot commented Oct 17, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 81e980e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4ac5b890-5d15-4de9-a01f-5bc7b74b2349

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@brianacnguyen brianacnguyen added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 17, 2024
@gambinish gambinish self-requested a review October 17, 2024 23:24
@brianacnguyen brianacnguyen requested review from a team as code owners October 17, 2024 23:44
@brianacnguyen brianacnguyen added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 17, 2024
Copy link
Contributor

github-actions bot commented Oct 17, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: c824377
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/558854b7-7a10-4cae-aabb-59ebb5e2f924

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@brianacnguyen brianacnguyen added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 18, 2024
Copy link
Contributor

github-actions bot commented Oct 18, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 94228f0
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4fe7a204-15d2-4d57-a276-14bbbe38ceb0

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@brianacnguyen brianacnguyen added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Oct 18, 2024
Copy link
Contributor

@gambinish gambinish left a comment

Choose a reason for hiding this comment

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

Pulled down, and looks good! The snapshot updates also look reasonable to me.

It looks like for certain networks, the left icon ... is non-functional. I wonder what the lift would be to not show that icon if it doesn't pop the BottomSheet when pressed?

Screen.Recording.2024-10-18.at.12.00.55.PM.mov

@brianacnguyen brianacnguyen requested review from a team as code owners October 18, 2024 22:15
@vinnyhoward
Copy link
Contributor

Pulled down, and looks good! The snapshot updates also look reasonable to me.

It looks like for certain networks, the left icon ... is non-functional. I wonder what the lift would be to not show that icon if it doesn't pop the BottomSheet when pressed?

Screen.Recording.2024-10-18.at.12.00.55.PM.mov

Everything looks good to me except this issue. I was able to reproduce it as well

@brianacnguyen
Copy link
Contributor Author

Pulled down, and looks good! The snapshot updates also look reasonable to me.

It looks like for certain networks, the left icon ... is non-functional. I wonder what the lift would be to not show that icon if it doesn't pop the BottomSheet when pressed?

Screen.Recording.2024-10-18.at.12.00.55.PM.mov

does this behavior currently exist in prod?

@gambinish
Copy link
Contributor

Pulled down, and looks good! The snapshot updates also look reasonable to me.
It looks like for certain networks, the left icon ... is non-functional. I wonder what the lift would be to not show that icon if it doesn't pop the BottomSheet when pressed?
Screen.Recording.2024-10-18.at.12.00.55.PM.mov

does this behavior currently exist in prod?

Technically idt no, because the previous header wouldn't display the overflow icon in the header at all, so it couldn't be pressed. I have a feeling that this might eventually come up as a regression. Any idea why the end accessory is being rendered here and not on main?

Screenshot 2024-10-21 at 11 47 57 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E Triggers smoke e2e on Bitrise team-design-system All issues relating to design system in Mobile
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

[Design quality]: Update the mobile header to the design system header - Token details
4 participants