[User Settings] Add new Preferences view #1716

Maftalion · 2021-03-11T00:54:07Z

Details

Add new toggle on the settings/preferences page to allow users to toggle their expensifyNewsStatus value

Fixed Issues

#1677

Tests

Go to profile by clicking your avatar
Click on preferences
Toggle and confirm expensifyNewsStatus is being updated

Tested On

Video

Screen.Recording.2021-03-09.at.3.48.10.PM.mov

Web

Mobile Web

Desktop

iOS

Android

src/pages/home/HomePage.js

This reverts commit fc03aee.

src/pages/settings/PreferencesPage.js

Maftalion · 2021-03-11T17:11:40Z

@shawnborton updated

shawnborton · 2021-03-11T17:36:58Z

Looks great! Perhaps we can make the margin below the first block slightly bigger to creates some more visual separation? Maybe like mb-6 or something?

bondydaa · 2021-03-11T20:39:22Z

src/pages/settings/PreferencesPage.js

-                        value={priorityMode}
-                        Icon={() => <Icon src={DownArrow} />}
-                    />
+class PreferencesPage extends Component {


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 with priorityMode?

The state you've added should be handled already by Onyx so as long as the isOn is bound to the prop user.expensifyNewsStatus I'm pretty sure it will update properly without the need for this to be a regular/pure component and reference state.

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.

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?

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.

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.

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!

src/pages/settings/PreferencesPage.js

# Conflicts: # config/webpack/webpack.common.js # src/pages/home/HomePage.js # src/pages/settings/PreferencesPage.js

NikkiWines

Tested and work well! No idea if this is something we can change but I noticed a funny little animation when we navigate to the Preferences page and you are subscribed to Expensify news.

Screen.Recording.2021-03-16.at.4.30.11.PM.mov

src/libs/Navigation/AppNavigator/AuthScreens.js

Maftalion · 2021-03-16T23:54:53Z

Tested and work well! No idea if this is something we can change but I noticed a funny little animation when we navigate to the Preferences page and you are subscribed to Expensify news.

@NikkiWines yeah, noticed that as well. It looks like it's initialized to "off" and gets animated to "on":
https://github.com/aminebenkeroum/toggle-switch-react-native/blob/master/ToggleSwitch.js#L89

but looking at this closer...it seems inefficient that it's doing everything in the render rather than watching for prop changes. it looks like a pretty simple component if we wanted to re-create it and modify it instead of using the package.

shawnborton · 2021-03-17T09:49:08Z

@Maftalion is there a way to customize how the toggle itself looks? In our mockups, we have the toggle at 50px wide by 28px tall, with the inner button at 22px by 22px.

Maftalion · 2021-03-17T14:08:40Z

@shawnborton yup, updated the styles on the track/thumb:

Maftalion · 2021-03-17T18:52:29Z

@roryabraham AFAIK the switch component isn't customizable enough to meet the design expectations: https://reactnative.dev/docs/switch. For example you can control thumb/track color but not it's height/width. Also you can't make all platforms render the "iOS switch".

But yeah what I'm suggesting is if we don't want to use the 3rd party library, we can just create a components/switch component and implement something similar that's pure JS that fits the needs of design parity across all platforms.

tgolen · 2021-03-17T21:23:27Z

we can just create a components/switch component and implement something similar that's pure JS that fits the needs of design parity across all platforms.

+💯

We should definitely do this. I don't think it will be very difficult at all. This helps to ensure consistent code patterns in our components which lowers the barrier of entry to other people who need to work on this.

shawnborton · 2021-03-18T08:40:24Z

@Maftalion I also wanted to confirm again if you know what's going on with the picker menus? It looks like this exists on the current app, so this might not be your doing, but the border radius is now set to 0 instead of matching other inputs:

Maftalion · 2021-03-18T15:53:36Z

@shawnborton hmm looks like it's using variables.componentBorderRadius which doesn't exist
https://github.com/Expensify/Expensify.cash/blob/master/src/styles/variables.js

# Conflicts: # src/styles/styles.js

Maftalion · 2021-03-18T16:39:22Z

Swapped to use the new Switch component, here's On/Off screenshots:

NikkiWines

One minor minor comment, but overall looking great! Tested on web and works well

src/components/Switch.js

shawnborton · 2021-03-19T09:11:31Z

@shawnborton hmm looks like it's using variables.componentBorderRadius which doesn't exist
https://github.com/Expensify/Expensify.cash/blob/master/src/styles/variables.js

@Maftalion do you mind adding that variable back in? It should have a value of 8

Maftalion · 2021-03-19T15:42:08Z

@shawnborton sure! added that back in:

shawnborton · 2021-03-19T15:45:02Z

Looks great, thank you!

config/webpack/webpack.common.js

# Conflicts: # src/libs/Navigation/AppNavigator/AuthScreens.js

NikkiWines

Looks good!

bondydaa

looks good overall just one thing I think we should do with the switch component.

src/components/Switch.js

bondydaa

actually I'm reversing course on my last comments, I think your points are valid and not worth the refactor to functional component with so many unknowns or potential drawbacks.

# Conflicts: # src/styles/styles.js

bondydaa · 2021-03-23T20:45:43Z

nikki had already approved and the only new changes are fixing merge conflicts so i'm going to merge. marc and I also chatted about this PR 1:1 so i'm considered that an implicit approval too. merging so we get wrap this up as i've caused it to be drawn out for way too long already.

Maftalion added 4 commits March 9, 2021 16:06

add expensifyNewsStatus toggle to preferences page

ca2cace

Merge branch 'master' into matt-preferences-toggle

ca98bf2

Merge branch 'master' into matt-preferences-toggle

1edbb1c

add toggle library, revert accidental change

a27b59d

Maftalion requested a review from a team as a code owner March 11, 2021 00:54

botify requested review from bondydaa and removed request for a team March 11, 2021 00:54

Maftalion commented Mar 11, 2021

View reviewed changes

src/pages/home/HomePage.js Outdated Show resolved Hide resolved

Maftalion added 3 commits March 10, 2021 16:59

package lock update

fc03aee

add proptype comments

f0475a0

Revert "package lock update"

8ffc704

This reverts commit fc03aee.

shawnborton reviewed Mar 11, 2021

View reviewed changes

src/pages/settings/PreferencesPage.js Outdated Show resolved Hide resolved

shawnborton reviewed Mar 11, 2021

View reviewed changes

src/pages/settings/PreferencesPage.js Outdated Show resolved Hide resolved

shawnborton reviewed Mar 11, 2021

View reviewed changes

src/pages/settings/PreferencesPage.js Outdated Show resolved Hide resolved

style/text updates

a1c53b7

bondydaa suggested changes Mar 11, 2021

View reviewed changes

style/small updates

abb27a2

NikkiWines assigned Maftalion Mar 15, 2021

NikkiWines linked an issue Mar 15, 2021 that may be closed by this pull request

[User Settings] Add new Preferences view #1677

Closed

Maftalion added 3 commits March 15, 2021 16:50

Merge branch 'master' into matt-preferences-toggle

7636f31

# Conflicts: # config/webpack/webpack.common.js # src/pages/home/HomePage.js # src/pages/settings/PreferencesPage.js

update to go based off onyx prop

dc4dbed

lint error

8703977

NikkiWines requested changes Mar 16, 2021

View reviewed changes

src/libs/Navigation/AppNavigator/AuthScreens.js Outdated Show resolved Hide resolved

add user fetch

b332485

update switch styles

fe50d3e

Maftalion added 2 commits March 18, 2021 09:32

Merge branch 'master' into matt-preferences-toggle

b7bd1da

# Conflicts: # src/styles/styles.js

create switch component, remove library

f33775d

lint

6732e6c

NikkiWines requested changes Mar 18, 2021

View reviewed changes

src/components/Switch.js Outdated Show resolved Hide resolved

Maftalion added 2 commits March 18, 2021 17:37

Merge branch 'master' into matt-preferences-toggle

dcec672

remove line

942c675

add missing style

ff754a6

Merge branch 'master' into matt-preferences-toggle

613b452

roryabraham reviewed Mar 19, 2021

View reviewed changes

config/webpack/webpack.common.js Outdated Show resolved Hide resolved

Maftalion added 4 commits March 19, 2021 17:26

Merge branch 'master' into matt-preferences-toggle

7acae50

Merge branch 'master' into matt-preferences-toggle

5f92090

# Conflicts: # src/libs/Navigation/AppNavigator/AuthScreens.js

remove package

cfc6c56

fix conflict

0976483

NikkiWines previously approved these changes Mar 20, 2021

View reviewed changes

bondydaa suggested changes Mar 22, 2021

View reviewed changes

src/components/Switch.js Show resolved Hide resolved

src/components/Switch.js Show resolved Hide resolved

src/components/Switch.js Show resolved Hide resolved

bondydaa previously approved these changes Mar 23, 2021

View reviewed changes

Merge branch 'master' into matt-preferences-toggle

41ae751

# Conflicts: # src/styles/styles.js

Maftalion dismissed stale reviews from bondydaa and NikkiWines via 41ae751 March 23, 2021 19:37

bondydaa approved these changes Mar 23, 2021

View reviewed changes

bondydaa merged commit 163efa2 into Expensify:master Mar 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[User Settings] Add new Preferences view #1716

[User Settings] Add new Preferences view #1716

Maftalion commented Mar 11, 2021 •

edited

Loading

Maftalion commented Mar 11, 2021

shawnborton commented Mar 11, 2021

bondydaa Mar 11, 2021

Maftalion Mar 12, 2021

bondydaa Mar 15, 2021

tgolen Mar 15, 2021

NikkiWines Mar 16, 2021

marcaaron Mar 16, 2021

NikkiWines left a comment

Maftalion commented Mar 16, 2021

shawnborton commented Mar 17, 2021

Maftalion commented Mar 17, 2021

Maftalion commented Mar 17, 2021 •

edited

Loading

tgolen commented Mar 17, 2021

shawnborton commented Mar 18, 2021

Maftalion commented Mar 18, 2021

Maftalion commented Mar 18, 2021

NikkiWines left a comment

shawnborton commented Mar 19, 2021

Maftalion commented Mar 19, 2021

shawnborton commented Mar 19, 2021

NikkiWines left a comment

bondydaa left a comment

bondydaa left a comment

bondydaa commented Mar 23, 2021

[User Settings] Add new Preferences view #1716

[User Settings] Add new Preferences view #1716

Conversation

Maftalion commented Mar 11, 2021 • edited Loading

Details

Fixed Issues

Tests

Tested On

Video

Web

Mobile Web

Desktop

iOS

Android

Maftalion commented Mar 11, 2021

shawnborton commented Mar 11, 2021

bondydaa Mar 11, 2021

Choose a reason for hiding this comment

Maftalion Mar 12, 2021

Choose a reason for hiding this comment

bondydaa Mar 15, 2021

Choose a reason for hiding this comment

tgolen Mar 15, 2021

Choose a reason for hiding this comment

NikkiWines Mar 16, 2021

Choose a reason for hiding this comment

marcaaron Mar 16, 2021

Choose a reason for hiding this comment

NikkiWines left a comment

Choose a reason for hiding this comment

Maftalion commented Mar 16, 2021

shawnborton commented Mar 17, 2021

Maftalion commented Mar 17, 2021

Maftalion commented Mar 17, 2021 • edited Loading

tgolen commented Mar 17, 2021

shawnborton commented Mar 18, 2021

Maftalion commented Mar 18, 2021

Maftalion commented Mar 18, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

shawnborton commented Mar 19, 2021

Maftalion commented Mar 19, 2021

shawnborton commented Mar 19, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

bondydaa left a comment

Choose a reason for hiding this comment

bondydaa left a comment

Choose a reason for hiding this comment

bondydaa commented Mar 23, 2021

Maftalion commented Mar 11, 2021 •

edited

Loading

Maftalion commented Mar 17, 2021 •

edited

Loading