-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Show Pop-over on Workspace Welcome #16962
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
Merged
Merged
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
c8951b4
Add optional parameter showPopoverMenu to Welcome.show
612317f
Call welcome show method to check for welcome popover when opening re…
78d1062
Bind showPopoverMenu to this so it updates the correct state
910cf23
Not take closed/archived workspace chats in consideration for new users
904cdbe
Linting Welcome
a3896c5
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
304abe3
Add missing trailing comma to ReportActionCompose
20bc260
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
29f9ad7
Add missing reference to CONST
ae34acf
Remove extra CONST import
576b196
Remove remaining reference to withwithNavigation
5d70fed
Add imports to Welcome and withNavigation removed during merge with main
b4bcbf0
Clear NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER after opening Popover menu…
7065e94
Update showPopoverMenu comments
9a1379c
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
b39d606
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
d21d32a
Refactor Welcome logic to control the state of isFirstTimeNewExpensif…
70db7a6
Only call Welcome.show at first-sign in from the correct enabled comp…
8f85ae5
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
4251d01
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
32f0299
Remove unecessary showPopoverMenu check
5974435
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
820fbaf
Add missing imports removed in last merge with main
ffb37e2
Merge branch 'main' of github.com:Expensify/App into paulogasparsv-wo…
2bc4c59
Remove mentions of state from stateless Welcome.js component comments
af0539e
Rephrase NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USER callback logic comment
64c4b7c
Remove last mention of state from Welcome.js comments
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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'm a bit confused, you mention in the comment above this block updates isFirstTimeNewExpensifyUser from true to false.
But for a new user, isFirstTimeNewExpensifyUser is first null from the top and then when backend returns true for
ONYXKEYS.NVP_IS_FIRST_TIME_NEW_EXPENSIFY_USERwon't this be set from null to true?Additionally from logic here if isFirstTimeNewExpensifyUser is true it will never set it to false. The condition here will give true only for
!falseso you can only change isFirstTimeNewExpensifyUser from false or null to true/false here.So maybe your comment needs to be rephrased. Let me know if I'm mistaken.
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 think my comment is very poorly written @chiragsalian so I'll explain each question and rework the comment too.
Yes, the first time this logic is reached is when we first set
isFirstTimeNewExpensifyUserto true.Yes! It only updates the value of
isFirstTimeNewExpensifyUserif it already was falsy.Since we never update
isFirstTimeNewExpensifyUserto true, we only setisFirstTimeNewExpensifyUserwhen it first loads through Onyx.