Login container center issue fixed #10158

metehanozyurt · 2022-07-30T07:24:07Z

Details

Login container center issue for small screen size fixed.

Fixed Issues

$ #9509

Tests

Go to Login page Expensify App
Decrease the screen width (height must be under 854 px )
Login container should be in the center between 800 and 1024

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Go to Login page Expensify App
Decrease the screen width (height must be under 854 px )
Login container should be in the center between 800 and 1024

Verify that no errors appear in the JS console

Screenshots

Web

web-login-container-Screen-Recording-2022-07-30-at-09.49.28.mp4

Mobile Web

Desktop

desktop-login-container-Screen-Recording-2022-07-30-at-09.52.08.mp4

iOS

Android

metehanozyurt · 2022-07-30T07:47:16Z

I feel bad for the line length and the controls getting longer. This is how I made my first commit to stay true to my proposal.
Do you want me to put a function like this under StyleUtils? Thanks.

/**
 * Gives the margin style for login container
 *
 * @param {Object} params
 * @returns {Object}
 */
function getLoginContainerMarginStyles({
    isSmallScreenWidth,
    isMediumScreenWidth,
    windowHeight,
}) {
    if (isSmallScreenWidth) {
        return styles.signInPageNarrowContentMargin;
    }
    if (!isMediumScreenWidth || (isMediumScreenWidth && windowHeight < variables.minHeightToShowGraphics)) {
        return styles.signInPageWideLeftContentMargin;
    }
    return {};
}

function call :

.....
contentContainerStyle={[
                    StyleUtils.getLoginContainerMarginStyles(props),
....

Santhosh-Sellavel · 2022-07-31T18:24:41Z

@metehanozyurt Good call, I too thought the same.
We should be good with this, so make the changes.

@sketchydroide If you feel otherwise let us know thanks.

Santhosh-Sellavel · 2022-08-02T19:30:46Z

@metehanozyurt Any update?

metehanozyurt · 2022-08-03T06:09:10Z

I updated the code. I thought I should wait for a second confirmation, sorry I misunderstood @Santhosh-Sellavel.

Santhosh-Sellavel · 2022-08-05T18:51:10Z

src/styles/StyleUtils.js

+    if (isSmallScreenWidth) {
+        return styles.signInPageNarrowContentMargin;
+    }
+    if (!isMediumScreenWidth || (isMediumScreenWidth && windowHeight < variables.minHeightToShowGraphics)) {
+        return styles.signInPageWideLeftContentMargin;
+    }
+    return {};
+}


This seems like an error can you recheck it, please?

For isSmallScreenWidth && styles.signInPageWideLeftContentMargin also should apply because !isMediumScreenWidth can be isSmallScreenWidth = true right?

Ohh You are absolutely right. I'm editing it as a spread operator in case other style types may come in the future.
If you disagree with this opinion, I can use the assignment operator right away.
Thank you so much for your warning of my mistake 🙏 .

I retake videos and screenshots immediately.

Can you share both snippets for better understanding, thanks!

version 1 :

let marginStyle = {}; if (isSmallScreenWidth) { marginStyle = {...marginStyle, ...styles.signInPageNarrowContentMargin}; } if (!isMediumScreenWidth || (isMediumScreenWidth && windowHeight < variables.minHeightToShowGraphics)) { marginStyle = {...marginStyle, ...styles.signInPageWideLeftContentMargin}; } return marginStyle;

version 2:

let marginStyle = {}; if (isSmallScreenWidth) { marginStyle = styles.signInPageNarrowContentMargin; } if (!isMediumScreenWidth || (isMediumScreenWidth && windowHeight < variables.minHeightToShowGraphics)) { marginStyle = styles.signInPageWideLeftContentMargin; } return marginStyle;

If there is a possibility of additional style information, I think the 1st method may be more useful in the future.

Version 2 is still invalid

I'm not writing to say that version 2 is beautiful. Version 2 also works because it has one variable. I think version 1 is more consistent for the future, of course.

signInPageNarrowContentMargin: { marginTop: '40%', }, signInPageWideLeftContentMargin: { marginTop: '44.5%', },

Santhosh-Sellavel · 2022-08-08T19:21:59Z

src/pages/signin/SignInPageLayout/SignInPageContent.js

@@ -55,8 +55,7 @@ const SignInPageContent = props => (
            <LoginKeyboardAvoidingView
                behavior="position"
                contentContainerStyle={[
-                    props.isSmallScreenWidth ? styles.signInPageNarrowContentMargin : {},
-                    !props.isMediumScreenWidth ? styles.signInPageWideLeftContentMargin : {},
+                    StyleUtils.getLoginContainerMarginStyles(props),


@metehanozyurt
On second thoughts, Let's revert the last two commits, just stick to the existing one itself.

props.isSmallScreenWidth ? styles.signInPageNarrowContentMargin : {}, !props.isMediumScreenWidth || (props.isMediumScreenWidth && props.windowHeight < variables.minHeightToShowGraphics) ? styles.signInPageWideLeftContentMargin : {},

I'm doing it right now. Thanks.

Santhosh-Sellavel · 2022-08-09T20:08:35Z

@metehanozyurt Can you resolve conflicts?

Santhosh-Sellavel · 2022-08-09T20:45:25Z

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Santhosh-Sellavel

@metehanozyurt
Tests well waiting to resolve conflicts!

cc: @sketchydroide

sketchydroide · 2022-08-10T10:00:51Z

I'll wait until you aprove as well @Santhosh-Sellavel and then we can merge this

Santhosh-Sellavel

LGTM! @sketchydroide

OSBotify · 2022-08-10T13:04:57Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Login container center issue fixed

91deb35

metehanozyurt requested a review from a team as a code owner July 30, 2022 07:24

melvin-bot bot requested review from Santhosh-Sellavel and sketchydroide and removed request for a team July 30, 2022 07:24

code has been simplified

30d8dcb

Santhosh-Sellavel suggested changes Aug 5, 2022

View reviewed changes

logic bug fixed

5eb6f4a

Santhosh-Sellavel reviewed Aug 8, 2022

View reviewed changes

code reverted

2dbe4fb

Santhosh-Sellavel suggested changes Aug 9, 2022

View reviewed changes

metehanozyurt added 2 commits August 10, 2022 08:48

Merge branch 'main' of https://github.com/Expensify/App into issue-9509

782ea4a

conflicts resolved

80bd6de

sketchydroide approved these changes Aug 10, 2022

View reviewed changes

Santhosh-Sellavel approved these changes Aug 10, 2022

View reviewed changes

sketchydroide merged commit eef92b9 into Expensify:main Aug 10, 2022

OSBotify mentioned this pull request Aug 15, 2022

Deploy Checklist: New Expensify 2022-08-04 #10253

Closed

kbecciv mentioned this pull request Aug 16, 2022

Desktop - Slide text at the right is not appearing when the login container is resized #10405

Closed

melvin-bot bot mentioned this pull request Aug 23, 2022

[HOLD for payment 2022-08-30] [$250] Web - Login container isn't in center @thesahindia #9509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Login container center issue fixed #10158

Login container center issue fixed #10158

metehanozyurt commented Jul 30, 2022

metehanozyurt commented Jul 30, 2022

Santhosh-Sellavel commented Jul 31, 2022 •

edited

Loading

Santhosh-Sellavel commented Aug 2, 2022

metehanozyurt commented Aug 3, 2022

Santhosh-Sellavel Aug 5, 2022

metehanozyurt Aug 5, 2022 •

edited

Loading

Santhosh-Sellavel Aug 5, 2022

metehanozyurt Aug 5, 2022

Santhosh-Sellavel Aug 8, 2022

metehanozyurt Aug 8, 2022

Santhosh-Sellavel Aug 8, 2022

metehanozyurt Aug 8, 2022

Santhosh-Sellavel commented Aug 9, 2022

Santhosh-Sellavel commented Aug 9, 2022

Santhosh-Sellavel left a comment

sketchydroide commented Aug 10, 2022

Santhosh-Sellavel left a comment

OSBotify commented Aug 10, 2022

Login container center issue fixed #10158

Login container center issue fixed #10158

Conversation

metehanozyurt commented Jul 30, 2022

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

metehanozyurt commented Jul 30, 2022

Santhosh-Sellavel commented Jul 31, 2022 • edited Loading

Santhosh-Sellavel commented Aug 2, 2022

metehanozyurt commented Aug 3, 2022

Santhosh-Sellavel Aug 5, 2022

Choose a reason for hiding this comment

metehanozyurt Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Santhosh-Sellavel Aug 5, 2022

Choose a reason for hiding this comment

metehanozyurt Aug 5, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel Aug 8, 2022

Choose a reason for hiding this comment

metehanozyurt Aug 8, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel Aug 8, 2022

Choose a reason for hiding this comment

metehanozyurt Aug 8, 2022

Choose a reason for hiding this comment

Santhosh-Sellavel commented Aug 9, 2022

Santhosh-Sellavel commented Aug 9, 2022

PR Reviewer Checklist

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

sketchydroide commented Aug 10, 2022

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

OSBotify commented Aug 10, 2022

Santhosh-Sellavel commented Jul 31, 2022 •

edited

Loading

metehanozyurt Aug 5, 2022 •

edited

Loading