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
Fix sign in page/set password page styles, refactoring #1910
Fix sign in page/set password page styles, refactoring #1910
Changes from 29 commits
1ed958a
91580d5
c2d8015
f6b1ef6
12b3938
fc2808e
de2ab03
09678ef
451bb16
47cf198
61a0a2f
436cfd1
42ca8e8
8ebaf84
6d56db5
f9e1fe6
9491840
9a362da
8db75cc
6b6c43f
88733cd
ea5b973
3330380
461aef6
8dc41d2
5bef9a1
f534f06
ade55fb
c7382e1
a658b53
cc0ebcd
e6a3604
951f4fb
5b48bd0
19684a4
c265e20
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.
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
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.
Ahhh that is strange and definitely not what I was going for. So the width used for
isSmallScreenWidth
also includes iPad widths.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.
@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.
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.
yes,
isSmallScreenWidth
is just based on the screen width on any platformI'm using deep links to open this screen directly like 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.
@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?
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.
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.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.
@marcaaron
Ah duh; I uploaded the wrong screenshot (I've got like 50 screenshots on my desktop, my bad!)
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.
@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).
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'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.