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: onboarding add pin extension screen #2347

Merged
merged 12 commits into from
May 1, 2023

Conversation

lujakob
Copy link
Contributor

@lujakob lujakob commented Apr 13, 2023

Describe the changes you have made in this PR

Add a pin extension screen as last step of onboarding.

Link this PR to an issue

Fixes #2343

Type of change

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

Screenshots of the changes [optional]

Bildschirmfoto 2023-04-13 um 15 41 46

How has this been tested?

Manually

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@lujakob
Copy link
Contributor Author

lujakob commented Apr 13, 2023

I added code to detect dark mode browser setting, taken from utils/index.ts getTheme which should detect a users settings

window.matchMedia("(prefers-color-scheme: dark)").matches

I changed my Chrome browser setting to dark mode. The welcome screen appears dark, but the mechanism to detect it as taken from getTheme does not work. I wonder why the rest of the screen still appears dark, if the code has no effect.

Anyways, if the code in getTheme does not work, I think it's a different issue. What do you think @reneaaron ?

@github-actions
Copy link

github-actions bot commented Apr 13, 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.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@reneaaron
Copy link
Contributor

reneaaron commented Apr 13, 2023

@lujakob You are on 🔥, great job!

Just tested it and worked fine for me. 💪 Some minor things I noticed during testing:

  • The images are quite small (you could add the full size images in the original issue or downsize them to the biggest size they can be displayed)
  • Shadows and/or rounding of the box seems a bit off, could you please double check that with the designs?

Will continue with the code review probably later today / tomorrow, but looks quite good!

@lujakob lujakob force-pushed the lujakob/onboarding-step-pin branch from 667655c to 0cc59c4 Compare April 13, 2023 20:35
@lujakob
Copy link
Contributor Author

lujakob commented Apr 13, 2023

@lujakob You are on 🔥, great job!

Just tested it and worked fine for me. 💪 Some minor things I noticed during testing:

  • The images are quite small (you could add the full size images in the original issue or downsize them to the biggest size they can be displayed)
  • Shadows and/or rounding of the box seems a bit off, could you please double check that with the designs?

Will continue with the code review probably later today / tomorrow, but looks quite good!

  • I overlooked the width class on the image when copy/pasting. I increased it now to w-80. I found the next size w-96 too big in proportion to the rest of the content. If you think it is still to small, please test w-96 and let me know.
  • I copy pasted the component from SetPassword component which does not have rounded corners and shadow. I applied them to the PinExtension and also to SetPassword to have it consistent.
  • I also added the puzzle icon. I think it fits quite well. (didn't notice it on the current component where it is already used)
  • I also added the "Alby" icon on point "3." - what do you think?
  • In case you wonder, I decided to add inline styles because the icons where slightly off. It didn't look good.

@lujakob lujakob force-pushed the lujakob/onboarding-step-pin branch from 0cc59c4 to 9d7c911 Compare April 13, 2023 20:46
@reneaaron reneaaron self-assigned this Apr 16, 2023
@lujakob
Copy link
Contributor Author

lujakob commented Apr 16, 2023

@reneaaron Thanks for reviewing. I resolved all issues.

@lujakob lujakob force-pushed the lujakob/onboarding-step-pin branch from 01a0b4f to 53a2103 Compare April 16, 2023 16:05
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.

2 little nitpicks, other than that: 💯

Thanks also for taking the time to rename some functions and introducing a new useTheme hook. 💪

tACK

@lujakob
Copy link
Contributor Author

lujakob commented Apr 18, 2023

2 little nitpicks, other than that: 💯

Thanks also for taking the time to rename some functions and introducing a new useTheme hook. 💪

tACK

Sounds good. I updated.

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.

Just tested another time, added some fixes regarding spacing. Great job! 💪

tACK

@lujakob
Copy link
Contributor Author

lujakob commented Apr 19, 2023

Just tested another time, added some fixes regarding spacing. Great job! 💪

tACK

Thanks for reviewing 🙏

@reneaaron
Copy link
Contributor

I removed the onboarding card now too, since those are kind of duplicate otherwise. cc @stackingsaunter

@reneaaron reneaaron merged commit 65f1c35 into getAlby:master May 1, 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.

Add "Pin your Alby Extension" screen as last onboarding step
3 participants