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: settings notifications #480

Merged
merged 1 commit into from
Sep 26, 2024
Merged

feat: settings notifications #480

merged 1 commit into from
Sep 26, 2024

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Sep 26, 2024

This PR implements screens for settings notifications, adds tabs for 'push' and 'email' notifications, and uses an input to store a preferred email address to receive notifications.

I used zod to validate the email address. I'll need design feedback on what to show for an invalid email address, rn I'm showing a toast but we only have a success and info types currently. I think I need to add a fail type?



@fbwoolf fbwoolf force-pushed the feat/settings-notifications branch 2 times, most recently from c27b12a to 02fde42 Compare September 26, 2024 00:51
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.38%. Comparing base (936fa26) to head (171db36).
Report is 9 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #480   +/-   ##
=======================================
  Coverage   22.38%   22.38%           
=======================================
  Files         132      132           
  Lines        5521     5521           
  Branches      241      241           
=======================================
  Hits         1236     1236           
  Misses       4285     4285           
Components Coverage Δ
bitcoin 53.77% <ø> (ø)
query 12.05% <ø> (ø)
utils 52.45% <ø> (ø)
crypto 69.40% <ø> (ø)
stacks 53.27% <ø> (ø)


return (
<Box flex={1} backgroundColor="ink.background-primary">
<ScrollView
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR but we seem to repeat this <ScrollView with these props a lot (5 times). We could look into a layout for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot I created a SettingsScreenLayout to use here, so will do. Thx for reminder.

Copy link
Contributor

@pete-watters pete-watters left a comment

Choose a reason for hiding this comment

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

Great work 🚀

I added suggestions but it's fine as is, bar typo

@edgarkhanzadian edgarkhanzadian added the needs:demo-build Create EAS simulator build based on label label Sep 26, 2024
@leather-bot
Copy link
Contributor

Copy link
Collaborator

@edgarkhanzadian edgarkhanzadian left a comment

Choose a reason for hiding this comment

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

Great work! A few notes here and there.

This back button first backs on tab navigation and then on stack navigation. I think the expected behavior here would be for the back button to just go back only on stack navigation:

navigationBack.mov

@fbwoolf fbwoolf force-pushed the feat/settings-notifications branch 2 times, most recently from 171db36 to 05e9656 Compare September 26, 2024 17:17
@leather-bot
Copy link
Contributor

@leather-bot
Copy link
Contributor

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Sep 26, 2024

Updated the toast types here cc @fabric-8, also the copy on the success was to wordy so I simplified to fit on screen better...

@leather-bot
Copy link
Contributor

@fbwoolf fbwoolf added this pull request to the merge queue Sep 26, 2024
Merged via the queue into dev with commit 498a1db Sep 26, 2024
11 checks passed
@fbwoolf fbwoolf deleted the feat/settings-notifications branch September 26, 2024 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:demo-build Create EAS simulator build based on label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants