[HOLD for payment 2022-11-02] [$250] Bug: Keyboard shortcut modal doesn't take whole width at the footer reported by @Puneet-here

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Change screen to medium screen size and press cmd + i
2. Change screen to small screen size and press cmd + i

Expected Result:

The modal should take the whole width or it should open in the middle

Actual Result:

The modal doesn't take the full width in medium screen size

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number:
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

<img width="722" alt="Screenshot 2022-09-27 at 8 25 29 PM" src="https://user-images.gi


thubusercontent.com/43996225/193888753-a569be4f-fd6c-4b36-9bfb-a25b26db06e0.png">

Expensify/Expensify Issue URL:
Issue reported by: @Puneet-here
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664291291091909

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[Puneet-here]

Proposal

we have maxWidth: 600 set in the keyboardShortcutModalContainer but we don't want that in small screen because at small screen the modal will be bottom docked so we can use this.props.isSmallScreenWidth to decide the style. We can create a variable like below for the styles and we can pass it to containerStyle

App/src/components/KeyboardShortcutsModal.js

Line 82 in 43b5092

containerStyle={styles.keyboardShortcutModalContainer}

const containerStyle = {...styles.keyboardShortcutModalContainer, ...this.props.isSmallScreenWidth? styles.mw100: {} }

Or we can also create a function at StyleUtils.js and pass screen size there to get the styles we want.

[Christinadobrzyn]

Ah interesting! This looks similar - #11411 - sending to eng to see if this should be a separate fix.

[melvin-bot]

Triggered auto assignment to @AndrewGable (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[AndrewGable]

It does seem like a separate issue, however, I would love @Expensify/design's confirmation that we want this modal to open full screen?

[michelle-thompson]

I don't think it should be full-screen, but full-width

[shawnborton]

Yeah, basically in this screenshot here:

The white modal part should extend to the edges of the device. Do we know why it is not? Just trying to understand if this has implications for other modals/bottom drawers in the app.

[Puneet-here]

@shawnborton, we are using maxWidth: 600 for this modal that's why it doesn't take the whole width at the bottom, we only need maxWidth: 600 at large screen so we can solve this by checking the screen size as proposed here

[shawnborton]

Sounds good, thanks for the explanation!

[melvin-bot]

@AndrewGable Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

Triggered auto assignment to @miljakljajic (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

[melvin-bot]

Current assignee @AndrewGable is eligible for the External assigner, not assigning anyone new.

[AndrewGable]

External issue

[rushatgabhane]

Or we can also create a function at StyleUtils.js and pass screen size there to get the styles we want

@Puneet-here sounds good, let's do that.

@AndrewGable I like @Puneet-here's proposal.
C+ reviewed 🎀 👀 🎀

[melvin-bot]

@AndrewGable, @miljakljajic, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

[miljakljajic]

Job posted to Upwork here.

[rushatgabhane]

awaiting @AndrewGable's review for #11588 (comment)

[AndrewGable]

👍

[Puneet-here]

@rushatgabhane, I am testing the code and capturing the screenshot but not sure what to do about mobile screenshots. This modal is only available for desktop app and web. I have taken the screenshots at mobile browser by changing the code but the mobile app crashes because of the styling here

App/src/styles/styles.js

Line 2579 in c638000

flex: '0 0 auto',

and I tried using the styles like this

        flexGrow: 0,
        flexShrink: 0,
        flexBasis: 'auto',

It solved the issue but I can't figure out why the modal looks like this

It's not related to the code change for this issue but wanted help for the screenshot (or should I leave the app screenshots since it can't be open at mobile app)

[rushatgabhane]

@Puneet-here it's okay to not add screenshots then. This feature isn't available on mobile

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.19-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: Keyboard shortcut modal doesn't take whole width at the footer #12071

If no regressions arise, payment will be issued on 2022-11-02. 🎊

[miljakljajic]

Thank you for handling this! I have extended hiring offers to both of you @Puneet-here and @rushatgabhane

[miljakljajic]

@Puneet-here & @rushatgabhane have been paid, contracts ended, job post closed. Thank you!