Fix sign in page/set password page styles, refactoring #1910

jasperhuangg · 2021-03-19T04:26:14Z

There were too many unrelated changes happening in this PR so @marcaaron and I decided to break this up into a bunch of smaller PRs. I'll update the links individually once each PR is created. For now, this PR won't be merged but will simply be broken up into these PRs:

PR for the flex styles Fix sign in page styles #2192 [MERGED]
PR for refactoring the welcome text into its own component Add WelcomeText component #2190 [MERGED]
PR for fixing link fonts Add font: fontFamily.GTA to link styles #2237 [MERGED]
PR for refactoring SigninPageLayout to be used with single component forms Fix signin page layout #2196 [MERGED]
PR for refactoring LoginForm to be a single component Refactor login form into a single component #2363 [MERGED]
PR for fixing styles for the SetPasswordPage (will be renamed to SetPasswordForm) Rename and restyle set password form #2211
PR for fixing CSS styles to allow native views to rely on width (as opposed to always defaulting to narrow).

shawnborton · 2021-03-19T09:33:52Z

Thanks @jasperhuangg! While we are here, I think it would be nice if the set password page looked more like the sign in page - same width, same centering, etc:

Desktop feels unnecessarily wide too, so perhaps we can fix that up as well?

jasperhuangg · 2021-03-22T04:14:08Z

Hey @shawnborton!

I rewrote the set password page to use an identical layout as the sign in page. I think it might look a bit too close, let me know what you think and if I need to get rid of anything. (see screenshots in the OP)

Just a note: if we got rid of the "With Expensify.cash, chat and payments are..." text and the screenshot I can consolidate everything for the SetPassword page into a single component.

cc @marcaaron

shawnborton · 2021-03-22T09:47:52Z

Nice, looks good to me design-wise. I like that we're just reusing the login/sign in styles.

marcaaron

Looks good, but I think we can maybe apply the DRY principle here. If possible, I'd love to see these changes all happening in one file (SetPasswordPage) without all of these new files.

src/pages/setpassword/SetPasswordForm/SetPasswordFormNarrow.js

src/pages/setpassword/SetPasswordForm/SetPasswordFormWide.js

src/pages/setpassword/SetPasswordForm/index.native.js

This reverts commit de2ab03.

jasperhuangg · 2021-03-24T01:15:42Z

Hey @marcaaron!

I DRYed up the code and I think it should be good for another review. The design didn't change from before (I think my previous commits dismissed Shawn's review), so feel free to merge if you think everything looks good!

src/pages/setpassword/SetPasswordPageProps.js

src/pages/setpassword/SetPasswordPage.js

src/pages/SetPasswordPage.js

marcaaron · 2021-03-24T01:41:51Z

feel free to merge if you think everything looks good

Hey @jasperhuangg there is a HOLD on this PR. If you remove it then it can be merged with an approval.

src/pages/setpassword/SetPasswordPage.js

src/pages/setpassword/SetPasswordPageProps.js

…nto jasper-fixSetPasswordPageFlex

marcaaron

Looks good just have a few more clean up suggestions.

src/pages/SetPasswordPage.js

src/components/WelcomeText.js

src/pages/SetPasswordPage.js

marcaaron

PR looks good, thanks for the changes. I found one inconsistency on iPad that we should maybe address here. Other than that, I'm unsure why there is a HOLD on this PR. It looks ready to merge after the iPad issue is fixed.

marcaaron · 2021-03-26T18:39:50Z

src/pages/SetPasswordPage.js

+                                    source={welcomeScreenshot}
+                                />
+                            </View>
+                        )}


Not sure if this is the effect you are going for, but I noticed that if we are viewing this on an iPad this "welcome screenshot" is no longer present.

We are using the "narrow" view on all iOS devices so this ends up looking like this...

iPad

iPhone

Ahhh that is strange and definitely not what I was going for. So the width used for isSmallScreenWidth also includes iPad widths.

@marcaaron I noticed that this issue appears when you try to run it natively on iOS; I was wondering how you were able to access the set password page on iOS in this branch? Normally for web I access it via the URL.

yes, isSmallScreenWidth is just based on the screen width on any platform

this issue appears when you try to run it natively on iOS

I'm using deep links to open this screen directly like this

npx uri-scheme open expensify-cash://setpassword/123456 --ios

@marcaaron Hmm tried it on my end with this branch and I wasn't able to reproduce the bug:

Is the main issue you're concerned about the image not showing up for you? Or did you want to change it so that we use the "wide" view for iPad?

The screenshot you are showing is the web version of the app shown in Safari app. The issue I am describing occurs in the native version of the app. Maybe try merging the main branch back into this branch and try again.

@marcaaron

Ah duh; I uploaded the wrong screenshot (I've got like 50 screenshots on my desktop, my bad!)

@marcaaron

So the issue is that we're defaulting to the narrow view on all iOS devices, but this iOS device in particular has a width greater than the cut off for mobile devices.

Since we put everything into a single component, I have to display the welcome screenshot conditionally based on the screen width.

I tried modifying index.native.js to use the screenWidth conditionally to display the narrow/wide views, but the wide view doesn't display correctly on iOS.

A solution is to refactor everything to use the same pattern as the log in page, or to figure out why the styles aren't showing up correctly for the wide view on iOS (which could potentially be a deep rabbit hole).

I've refactored everything back into multiple components seeing that the sign in page follows this same pattern, and it makes more sense to have two pages that look nearly identical to have the same structure. This is necessary for everything to display properly seeing that we default to the narrow view for ios.

…ices.

shawnborton · 2021-03-31T07:16:24Z

Jumping in late here, but we should be using screen width to determine the layout and not the operating system. Is that what we're doing here?

marcaaron

Sorry @jasperhuangg I feel like this PR has regressed back to where we started and we're now going in circles. Things were very close, then we took a few steps backwards. Not sure why.

src/libs/Navigation/AppNavigator/PublicScreens.js

src/pages/SetPasswordPage.js

src/pages/setpassword/SetPasswordForm.js

src/pages/setpassword/SetPasswordPageNarrow.js

marcaaron · 2021-03-31T18:17:41Z

Jumping in late here, but we should be using screen width to determine the layout and not the operating system. Is that what we're doing here?

+1 I'm not following why we are always defaulting to a "narrow" view on native platforms, but maybe there is a clear reason for it? To me, the existing design of the LoginForm makes things complicated since we are presenting only the "narrow" view on native platforms which forces us to decentralize various things. If everyone agrees, I think we should ditch this pattern and not continue to follow it for the next screen in the sequence (set password).

We can also create a follow up to address that concern, but let's come to a decision before proceeding further.

This reverts commit cc0ebcd.

…tive devices." This reverts commit a658b53.

… can be used with components that don't have wide/narrow counterparts.

added flex: 1 to set password page

1ed958a

jasperhuangg requested a review from a team as a code owner March 19, 2021 04:26

jasperhuangg self-assigned this Mar 19, 2021

botify requested review from marcaaron and removed request for a team March 19, 2021 04:26

Make set password page UI more similar to sign in page UI

91580d5

jasperhuangg changed the title ~~Added flex: 1 to sign in page styles~~ Make set password page styles like sign in page Mar 22, 2021

jasperhuangg requested a review from shawnborton March 22, 2021 04:10

fix eslint

c2d8015

jasperhuangg changed the title ~~Make set password page styles like sign in page~~ [HOLD Expensify.cash 1857] Make set password page styles like sign in page Mar 22, 2021

shawnborton previously approved these changes Mar 22, 2021

View reviewed changes

marcaaron requested changes Mar 22, 2021

View reviewed changes

src/pages/setpassword/SetPasswordForm/SetPasswordFormNarrow.js Outdated Show resolved Hide resolved

src/pages/setpassword/SetPasswordForm/SetPasswordFormWide.js Outdated Show resolved Hide resolved

src/pages/setpassword/SetPasswordForm/index.native.js Outdated Show resolved Hide resolved

dry up and reduce the number of components

f6b1ef6

jasperhuangg dismissed shawnborton’s stale review via f6b1ef6 March 23, 2021 11:47

jasperhuangg added 2 commits March 23, 2021 20:00

fix styles

12b3938

fix jsx style

fc2808e

jasperhuangg requested a review from marcaaron March 23, 2021 12:01

jasperhuangg added 3 commits March 24, 2021 09:07

remove accidentally committed files

de2ab03

Revert "remove accidentally committed files"

09678ef

This reverts commit de2ab03.

revert to master versions

451bb16

marcaaron requested changes Mar 24, 2021

View reviewed changes

jasperhuangg added 3 commits March 24, 2021 18:29

refactor based on PR comments

47cf198

Update WelcomeText.js

61a0a2f

remove unused eslint-disable

436cfd1

put with onyx back

8db75cc

jasperhuangg requested a review from marcaaron March 24, 2021 12:29

jasperhuangg added 5 commits March 25, 2021 15:59

Refactor everything into a single component

6b6c43f

Merge branch 'master' into jasper-fixSetPasswordPageFlex

88733cd

Eslint fix

ea5b973

Merge remote-tracking branch 'origin/jasper-fixSetPasswordPageFlex' i…

3330380

…nto jasper-fixSetPasswordPageFlex

Fix import

461aef6

marcaaron requested changes Mar 25, 2021

View reviewed changes

jasperhuangg added 5 commits March 26, 2021 14:02

Use compose

8dc41d2

Add displayName

5bef9a1

Use ...windowDimensionsPropTypes

f534f06

Revert stuff changed by --fix

ade55fb

Revert stuff changed by --fix

c7382e1

jasperhuangg requested a review from marcaaron March 26, 2021 06:08

marcaaron reviewed Mar 26, 2021

View reviewed changes

jasperhuangg changed the title ~~[HOLD Expensify.cash 1857] Make set password page styles like sign in page~~ Make set password page styles like sign in page Mar 29, 2021

jasperhuangg added 2 commits March 31, 2021 13:16

Revert back into multiple components to fix layout on wide native dev…

a658b53

…ices.

Fix ESLint

cc0ebcd

jasperhuangg requested a review from marcaaron March 31, 2021 05:22

marcaaron requested changes Mar 31, 2021

View reviewed changes

jasperhuangg added 5 commits April 1, 2021 09:37

Revert "Fix ESLint"

e6a3604

This reverts commit cc0ebcd.

Revert "Revert back into multiple components to fix layout on wide na…

951f4fb

…tive devices." This reverts commit a658b53.

Refactored back into single components, change SigninPageLayout so it…

5b48bd0

… can be used with components that don't have wide/narrow counterparts.

Fix indent

19684a4

Remove duplicate text

c265e20

jasperhuangg changed the title ~~Make set password page styles like sign in page~~ Fix sign in page/set password page styles, refactoring Apr 1, 2021

jasperhuangg closed this Apr 1, 2021

jasperhuangg mentioned this pull request Apr 6, 2021

Add font: fontFamily.GTA to link styles #2237

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sign in page/set password page styles, refactoring #1910

Fix sign in page/set password page styles, refactoring #1910

jasperhuangg commented Mar 19, 2021 •

edited

Loading

shawnborton commented Mar 19, 2021

jasperhuangg commented Mar 22, 2021 •

edited

Loading

shawnborton commented Mar 22, 2021

marcaaron left a comment

jasperhuangg commented Mar 24, 2021

marcaaron commented Mar 24, 2021

marcaaron left a comment

marcaaron left a comment

marcaaron Mar 26, 2021

jasperhuangg Mar 29, 2021 •

edited

Loading

jasperhuangg Mar 29, 2021

marcaaron Mar 29, 2021

jasperhuangg Mar 30, 2021

marcaaron Mar 30, 2021

jasperhuangg Mar 31, 2021 •

edited

Loading

jasperhuangg Mar 31, 2021

jasperhuangg Mar 31, 2021

shawnborton commented Mar 31, 2021

marcaaron left a comment

marcaaron commented Mar 31, 2021 •

edited

Loading

Fix sign in page/set password page styles, refactoring #1910

Fix sign in page/set password page styles, refactoring #1910

Conversation

jasperhuangg commented Mar 19, 2021 • edited Loading

shawnborton commented Mar 19, 2021

jasperhuangg commented Mar 22, 2021 • edited Loading

shawnborton commented Mar 22, 2021

marcaaron left a comment

Choose a reason for hiding this comment

jasperhuangg commented Mar 24, 2021

marcaaron commented Mar 24, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron Mar 26, 2021

Choose a reason for hiding this comment

jasperhuangg Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

jasperhuangg Mar 29, 2021

Choose a reason for hiding this comment

marcaaron Mar 29, 2021

Choose a reason for hiding this comment

jasperhuangg Mar 30, 2021

Choose a reason for hiding this comment

marcaaron Mar 30, 2021

Choose a reason for hiding this comment

jasperhuangg Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

jasperhuangg Mar 31, 2021

Choose a reason for hiding this comment

jasperhuangg Mar 31, 2021

Choose a reason for hiding this comment

shawnborton commented Mar 31, 2021

marcaaron left a comment

Choose a reason for hiding this comment

marcaaron commented Mar 31, 2021 • edited Loading

jasperhuangg commented Mar 19, 2021 •

edited

Loading

jasperhuangg commented Mar 22, 2021 •

edited

Loading

jasperhuangg Mar 29, 2021 •

edited

Loading

jasperhuangg Mar 31, 2021 •

edited

Loading

marcaaron commented Mar 31, 2021 •

edited

Loading