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

feat: add account detail pages header #2538

Merged
merged 18 commits into from
Jul 24, 2023

Conversation

lujakob
Copy link
Contributor

@lujakob lujakob commented Jul 5, 2023

Describe the changes you have made in this PR

Add header with account name to account detail pages.

Link this PR to an issue [optional]

Fixes #2521

Type of change

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

Bildschirmfoto 2023-07-05 um 07 33 43

How has this been tested?

Manually

Checklist

  • Self-review of changed code
  • Manual testing
  • Added automated tests where applicable
  • Update Docs & Guides
  • For UI-related changes
  • Darkmode
  • Responsive layout

@lujakob
Copy link
Contributor Author

lujakob commented Jul 5, 2023

@reneaaron please have a look. Not sure if you addressed all desired pages.

  • I created an AccountDetailHeader component
  • I added a cancel button to the generate page, as the import page had one as well
  • I changed the loading state for the backup page and added a backup.no_mnemonic_found, because when I visited this page via typing the url there was a continuous loading state, due to the mnemonic data being null.

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

Don't forget: keep earning sats!

@reneaaron
Copy link
Contributor

@lujakob Thanks for the PR, I refactored it to use subroutes / layouts instead of implementing the same things over and over on different pages.

I added a cancel button to the generate page, as the import page had one as well

Cool, thanks.

I changed the loading state for the backup page and added a backup.no_mnemonic_found, because when I visited this page via typing the url there was a continuous loading state, due to the mnemonic data being null.

I don't think we need to handle that and removed it.

Please have a look 😊

@lujakob
Copy link
Contributor Author

lujakob commented Jul 6, 2023

@lujakob Thanks for the PR, I refactored it to use subroutes / layouts instead of implementing the same things over and over on different pages.

I added a cancel button to the generate page, as the import page had one as well

Cool, thanks.

I changed the loading state for the backup page and added a backup.no_mnemonic_found, because when I visited this page via typing the url there was a continuous loading state, due to the mnemonic data being null.

I don't think we need to handle that and removed it.

Please have a look 😊

Looks good, thanks for helping out.

Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Addressed review comments and improved "back" navigation.

src/app/components/AccountDetailLayout/index.tsx Outdated Show resolved Hide resolved
src/app/components/AccountDetailLayout/index.tsx Outdated Show resolved Hide resolved
@stackingsaunter
Copy link
Contributor

stackingsaunter commented Jul 11, 2023

I propose changes to the accounts header to make it smaller and take less space and move "export" ino settings as an item (now known as "Connect to mobile wallet"

image

Zeus links to: https://zeusln.app/
Blue Wallet links to: https://bluewallet.io/

Figma designs and handoff

This would require some copy changes:

  • "Edit Account" header of the section to just "Account" (not ideal, maybe you have better propositions?)
  • "Export Account" modal name to "Connect to mobile wallet"
  • "Name" to "Display Name"

@reneaaron reneaaron marked this pull request as draft July 14, 2023 13:00
@rolznz
Copy link
Contributor

rolznz commented Jul 24, 2023

@reneaaron why did this get marked as a draft? is it still planned to be worked on?

@reneaaron
Copy link
Contributor

reneaaron commented Jul 24, 2023

@rolznz I noticed some problems during the final tests:

When you change your account name it's not updated in the account header (but it is updated in the account dropdown). Let me know if you have any ideas how to fix that. Need to take another look at that...

@reneaaron reneaaron closed this Jul 24, 2023
@reneaaron reneaaron reopened this Jul 24, 2023
@lujakob
Copy link
Contributor Author

lujakob commented Jul 24, 2023

@rolznz I noticed some problems during the final tests:

When you change your account name it's not updated in the account header (but it is updated in the account dropdown). Let me know if you have any ideas how to fix that. Need to take another look at that...

I see the name displayed in the AccountLayoutPage is fetched on component mount via the parameter id.

https://github.com/lujakob/lightning-browser-extension/blob/ad5ef0d234c7e00f82a301c29d43769a90812362/src/app/components/AccountDetailLayout/index.tsx#L22C1-L23

It is updated in the account header only when the account name being changed is the currently selected account. Which makes sense.

The parent AccountLayoutPage component where the name is displayed has no way of knowing when the child AccountDetail updates the name. So the only way I can think of right now is, reloading the page after updating the name, which is kinda dirty :(

@reneaaron reneaaron marked this pull request as ready for review July 24, 2023 13:51
@reneaaron reneaaron requested a review from rolznz July 24, 2023 13:52
@lujakob
Copy link
Contributor Author

lujakob commented Jul 24, 2023

@rolznz I noticed some problems during the final tests:
When you change your account name it's not updated in the account header (but it is updated in the account dropdown). Let me know if you have any ideas how to fix that. Need to take another look at that...

I see the name displayed in the AccountLayoutPage is fetched on component mount via the parameter id.

https://github.com/lujakob/lightning-browser-extension/blob/ad5ef0d234c7e00f82a301c29d43769a90812362/src/app/components/AccountDetailLayout/index.tsx#L22C1-L23

It is updated in the account header only when the account name being changed is the currently selected account. Which makes sense.

The parent AccountLayoutPage component where the name is displayed has no way of knowing when the child AccountDetail updates the name. So the only way I can think of right now is, reloading the page after updating the name, which is kinda dirty :(

I saw you found a good solution, Rene, perfect.

@rolznz rolznz merged commit 6c7db4d into getAlby:master Jul 24, 2023
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.

[Feature] Display profile header for account related pages
5 participants