Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
UI - onboarding styling and colors #131
UI - onboarding styling and colors #131
Changes from 25 commits
8ca66a9
ea5109e
235faa0
d206422
b330dd1
6fcd2ff
ffae221
2d77310
74811ae
cf19364
835cb44
7e5f4c8
ae0d622
bff0ab9
1f14532
c2ca8e0
0af1285
0ed1b50
baebfee
1e16bf0
f79a99c
f71549c
8bb5f61
08199e6
7c6e8e2
6e685df
13f649b
51261a7
be68e0a
49a4391
cda09fe
3db9ac0
15451ed
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change requested: I like @observable more too, but since we AppStorage and @observable dont play nice together I see why you did what you did.
however I'd prefer to leave this view model using
ObservableObject
and@AppStorage("isOnboarding") var isOnboarding: Bool?
for the moment just because we do the same thing in 2 other view models, so if we make a change to this one we should make a change to the other 2, and for the moment I'd prefer we just keep things as is to keep this PR focused (unless you just couldn't get things to work with ObservableObject at all? I know you mentioned something about ObservableObject related to this PR and maybe something not working quite right with passing theisOnboarding
between views or something, so let me know if ObservableObject just didn't work for you anymore here and maybe we can figure out a reason/alternative?)I do like using
@Observable
and I also like usingAppStorage
so we will have to come back to this in another PR to get those two working together.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to switch back to @observableobject, clearly the errors I encountered previously were user errors.
This minimises the changes for this PR.
I do wonder however for the future if we can find another pattern to rely on when knowing if the app should be in onboarding mode or not, other than calling Appstorage from so many different files. Some options:
Here's what I did in BDG Wallet: