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

Add accordion to settings page #361

Merged
merged 11 commits into from
Sep 25, 2023
Merged

Add accordion to settings page #361

merged 11 commits into from
Sep 25, 2023

Conversation

Lykhoyda
Copy link
Collaborator

@Lykhoyda Lykhoyda commented Sep 22, 2023

Closes #336

  • Update UI of the Settings page (Watched account and Wallet connect)
  • Watched accounts moved from components to the Settings page folder (as used now only in one place)
  • Feature to expand accordion using hashtags #watched-acccounts and #wallet-connect

!
There is no Wallet connect design yet

@asnaith
Copy link
Member

asnaith commented Sep 22, 2023

This is a nice improvement, especially on mobile. Just a couple of feedback points:

CleanShot 2023-09-22 at 14 47 56@2x

  1. I think it would be better if the "Currently watched accounts" label is only displayed when there are accounts being watched
  2. I know this is on the Figma design but it seems unnecessary to have this label as it's a duplication of the accordion title above it

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

This looks great!! I've a couple small changes that I'll push, like for the WalletConnect icon. I'll do it later today.

The links to setting > watched account need to be modified in the home page (and maybe some other places?) with the #panel-watched-accounts. This should make the Cypress test pass, or get us closer to.

packages/ui/src/pages/Settings/Settings.tsx Outdated Show resolved Hide resolved
packages/ui/src/pages/Settings/Settings.tsx Outdated Show resolved Hide resolved
packages/ui/src/pages/Settings/Settings.tsx Outdated Show resolved Hide resolved
packages/ui/src/pages/Settings/Settings.tsx Outdated Show resolved Hide resolved
packages/ui/src/pages/Settings/Settings.tsx Outdated Show resolved Hide resolved
packages/ui/src/pages/Settings/WatchedAccounts.tsx Outdated Show resolved Hide resolved
@Lykhoyda
Copy link
Collaborator Author

This looks great!! I've a couple small changes that I'll push, like for the WalletConnect icon. I'll do it later today.

The links to setting > watched account need to be modified in the home page (and maybe some other places?) with the #panel-watched-accounts. This should make the Cypress test pass, or get us closer to.

We need to add click on accordion for cypress test as before the inputs appeared right away.

@Lykhoyda Lykhoyda requested a review from Tbaut September 22, 2023 15:35
@Lykhoyda
Copy link
Collaborator Author

@asnaith @Tbaut please take a look again. I fixed the comments, let me know if i missed something

@asnaith
Copy link
Member

asnaith commented Sep 22, 2023

@asnaith @Tbaut please take a look again. I fixed the comments, let me know if i missed something

@Lykhoyda Those updates for the labels look good 👍

We need to add click on accordion for cypress test as before the inputs appeared right away.

If you can update the link for the button below to use settings#watched-acccounts then it's going to be expanded when arriving on that screen and will be better for users and also no need to update the test

CleanShot 2023-09-22 at 17 27 29@2x

@Tbaut
Copy link
Collaborator

Tbaut commented Sep 24, 2023

I pushed some changes to

  • change the logic to force open only the watched-account section if needed.
  • optimize the WC SVG that took a while to load
  • make the tests pass. Please check it out. We won't merge any PR that doesn't pass the test, unless it's due to flakiness (not the case here).

There are still a couple comments above that weren't taken into account yet, but it should be quick.

@Tbaut Tbaut changed the title Lykhoyda/update settings page Add accordion to settings page Sep 25, 2023
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

awesome, this looks a million time better. Thank you!

@Tbaut Tbaut merged commit a0df30e into main Sep 25, 2023
@Tbaut Tbaut deleted the lykhoyda/update_settings_page branch September 25, 2023 13:06
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.

Rework Settings view to use accordion
3 participants