login form layout for ipad #8752
Conversation
melvin-bot
bot
requested review from
iwiznia and
Santhosh-Sellavel
and removed request for
a team
| resizeMode={props.isMediumScreenWidth ? 'contain' : 'cover'} | ||
| /> | ||
| </Pressable> | ||
| {graphicLayout(props.isMediumScreenWidth)} |
There was a problem hiding this comment.
why props.isMediumScreenWidth is passed here?
| const graphicLayout = () => ( | ||
| <Pressable | ||
| style={[ | ||
| styles.flex1, | ||
| StyleUtils.getBackgroundColorStyle(backgroundStyle.backgroundColor), | ||
| ]} | ||
| onPress={() => { | ||
| Link.openExternalLink(backgroundStyle.redirectUri); | ||
| }} | ||
| disabled={!hasRedirect} | ||
| > | ||
| <SVGImage | ||
| width="100%" | ||
| height="100%" | ||
| src={backgroundStyle.backgroundImageUri} | ||
| resizeMode={props.isMediumScreenWidth ? 'contain' : 'cover'} | ||
| /> | ||
| </Pressable> | ||
| ); |
There was a problem hiding this comment.
Should this be an arrow function? I think it's not necessary
There was a problem hiding this comment.
@Santhosh-Sellavel oh yes, you're right arrow function isn't necessary, also passing props.isMediumScreenWidth isn't necessary too as it is in the props so removing it where it is passed in props.
|
@Santhosh-Sellavel please review pushed the changes |
|
@iwiznia Can you initiate the workflow? |
|
@aneequeahmad Resized Desktop app is broken Floating window
Split Window
cc: @iwiznia |
|
@Santhosh-Sellavel will be updating after pushing the commit. Thanks |
|
@Santhosh-Sellavel can't repro Resized desktop app broken issue. |
|
Oh reproduced and fixed the issues, screenshots attached after fix Floating window
Split Window |
config/webpack/webpack.dev.js
Outdated
| @@ -27,6 +27,7 @@ module.exports = (env = {}) => { | |||
| devServer: { | |||
| contentBase: path.join(__dirname, '../../dist'), | |||
| hot: true, | |||
| host: '0.0.0.0', | |||
There was a problem hiding this comment.
Remove this, it doesn’t need to be committed!
You can pass host to the webpack by just adding --host 0.0.0.0 to the yarn web command.
|
@Santhosh-Sellavel oh yes this shouldn't be pushed. Removed this line |
|
Update - i initiated the GH workflow as requested by this comment to get it moving along. |
|
@aneequeahmad Screen.Recording.2022-04-29.at.6.40.53.AM.mov |
|
@iwiznia We have one edge case here, I am trying to resize the web/desktop app. Which causes this issue -> #8752 (comment) Let's check with the design what to do for this view. There is not enough height available, we should consider both height & width ratios to decide on the layout. cc: @megankelso Since gave the wireframe here -> #8122 (comment) |
|
@Expensify/design can you take a look at the above please? |
|
This side looks broken: Notice how the orange background extends over top of the bottom controls of the gray half. |
|
Should we do away with the graphic completely? Even when the graphic isn't overextending as @shawnborton mentioned, it still feels a little awkward and broken to me. |
|
That's a good point - maybe we want to have a minimum height requirement here and if it's not met, don't show the bottom graphic? I think the original goal of this issue was that for iPad screens in vertical mode, the screen is super tall so we had plenty of space for both the login form and the graphic. In the case that's mocked up above that I pointed out, the screen height isn't actually that tall and thus it all feels a bit squished. |
Sounds good to me! |
|
@Santhosh- I think we can limit this UI implemented for a specific range of screen sizes(tablets and ipads). Should we do these design changes in this PR ? |
|
Yeah, it would be good if we do that. |
|
@Santhosh-Sellavel Pushed the changes as requested. Thanks cc: @iwiznia (Great finding thanks) |
| @@ -55,7 +55,8 @@ const SignInPageContent = props => ( | |||
| <LoginKeyboardAvoidingView | |||
| behavior="position" | |||
| contentContainerStyle={[ | |||
| props.isSmallScreenWidth ? styles.signInPageNarrowContentMargin : styles.signInPageWideLeftContentMargin, | |||
| props.isSmallScreenWidth ? styles.signInPageNarrowContentMargin : {}, | |||
| !props.isMediumScreenWidth && styles.signInPageWideLeftContentMargin, | |||
There was a problem hiding this comment.
The comment I left for the line above also applied to this one
|
@iwiznia Done. |
|
@iwiznia Let's merge this one!! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@aneequeahmad Can you update QA steps for this one? Do we need to test it? |
|
Oh crap, I'm not sure how did I miss that. @mvtglobally H Here are the steps.
|
|
@mvtglobally Updated the steps. |
|
Hey @Expensify/applause, could we get this one tested please using these steps. Thanks! |
|
@Julesssss Can you confirm the build we should use for validation? |
|
Hi @mvtglobally, could you please test against Update: sorry @mvtglobally I see now that the same test is going to fail. I'll let you know when we're ready for further testing. |
|
Morning @Expensify/applauseleads, this one is ready for retesting now. Build |
|
@Julesssss Here are the screens. Let us know if this is pass or fail as team is not fully sure |
|
Hi @mvtglobally, thanks! We should expect medium-sized tablets to not show the horizontal divider. So that seems good to me 👍 If we decide that the hardcoded cutoff height needs to change, that should be treated as a separate, non-deploy blocking issue. |
|
🚀 Deployed to production by @Julesssss in version: 1.1.65-6 🚀
|
Details
Fixed Issues
#8122
Tests
For ipad/tablets
For other platforms
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
For ipad/tablets*
For other platforms
Screenshots
Web
Desktop
IOS
Android
Mweb
IPad