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
[User Settings] Add new Preferences view #1716
[User Settings] Add new Preferences view #1716
Changes from 7 commits
ca2cace
ca98bf2
1edbb1c
a27b59d
fc03aee
f0475a0
8ffc704
a1c53b7
abb27a2
7636f31
dc4dbed
8703977
b332485
fe50d3e
802d076
ddc8723
f48421d
67bba32
38f1bc1
6c4f7c3
a4e0640
b7bd1da
f33775d
6732e6c
dcec672
942c675
ff754a6
613b452
7acae50
5f92090
cfc6c56
0976483
41ae751
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
why did you convert this from a functional component? couldn't we keep it as a functional component and pass
user
as a prop like we were doing before withpriorityMode
?The state you've added should be handled already by Onyx so as long as the
isOn
is bound to the propuser.expensifyNewsStatus
I'm pretty sure it will update properly without the need for this to be a regular/pure component and reference state.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.
Hey, so this was the direction I went down to follow the "offline first" approach. Basically if it was going purely based off the
user.expensifyNewsStatus
then it would seem unresponsive/slow for users with no or slow connections. I guess a nice follow up to handle the errors would be to show a toast & reset back to the initial onyx value but I believe that's out of scope.I can also revert it back to a func component and go solely off the prop if needed.
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.
Hmm I see that does make sense now but that makes me question whether the
User.setExpensifyNewsStatus
should first be setting the data in onyx and then calling the API.cc @tgolen what are your thoughts on this?
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.
I like the idea of optimistically setting the value in Onyx before calling the API, yeah. Then if the API fails, the Onyx data can be reset.
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.
Hmm, I'm not sure I agree with this, as I think we're going to end up with a lot of duplicated code with having to handle the case where we don't get a response at all and when we get a failed response (see here). I get that we want to keep that offline first functionality though, so I'm not really sure which is the best route to go.
@marcaaron this didn't get brought up when I first added
User.setExpensifyNewsStatus
so I'm curious to see if you have opinions on this.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.
I like the optimistic approach here as well. Only thought is that we should consider updating the value of this data with the data returned from the response instead of relying purely on response codes - but it's probably OK with this particular setting being a boolean and all.
I'm also inclined to wait and see how many examples like this we end up with (though I agree it will be a lot eventually). Would love to see a more elegant or baked in approach to this optimistic response pattern soon!