Skip to content
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

5009 mobile modal improvements #6065

Draft
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

lache-melvin
Copy link
Contributor

Fixes #5009

Leaving in draft until I do a full sweep of the modals...

πŸ‘©πŸ»β€πŸ’» What does this PR do?

Full screen modals! Also fixes autocomplete dropdowns going crazy places and some other funchy behaviour!

Screenshot 2025-01-15 at 4 57 18β€―PM

πŸ’Œ Any notes for the reviewer?

Please give her a thorough sweep πŸ˜…

Few threads solved here, will put comments throughout

πŸ§ͺ Testing

  • Browser/desktop modals are working as expected
  • On android
    • The bigger input modals are all in full screen mode
    • Dropdowns on android don't jump to strange places when keyboard comes up

πŸ“ƒ Documentation

  • Part of an epic: documentation will be completed for the feature as a whole
  • No documentation required: no user facing changes or a bug fix which isn't a change in behaviour
  • These areas should be updated or checked:
    1. Maybe worth mentioning somewhere that modals look different on android?

@github-actions github-actions bot added this to the v2.6.0 milestone Jan 15, 2025
@github-actions github-actions bot added Priority: Must Have The product will not work without this refactor Team Korimako LachΓ©, Aimee, Noel John, and Zachariah labels Jan 15, 2025
@lache-melvin
Copy link
Contributor Author

lache-melvin commented Jan 15, 2025

will upload an APK to test when ready

marginBottom: '30px',
marginTop: '30px',
marginBottom: keyboardIsOpen ? 0 : '30px',
marginTop: keyboardIsOpen ? 0 : '30px',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes the margins around the buttons smaller while using the keyboard (because they take up a lot of space)

@@ -161,6 +166,9 @@ export const useDialog = (dialogProps?: DialogProps): DialogState => {
isRtl,
animationTimeout
);
const { isOpen: keyboardIsOpen } = useKeyboardContext();
const isSmallerScreen = useMediaQuery('(max-height: 850px)');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Felt like a reasonable height to be okay with sticking with modals as they are, most tablets around 750-800 high... happy to play around with this

>
{fullScreen && (
<IconButton
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some modals used clicking on the backdrop to close (e.g. internal order create) so added close button

Copy link

github-actions bot commented Jan 15, 2025

Bundle size difference

Comparing this PR to main

Old size New size Diff
5.15 MB 5.15 MB 1.36 KB (0.03%)

isOpen,
onClose,
disableMobileFullScreen: true,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input modal is small (used for like tax input) so don't need the full screen capability here

return (
<Box sx={{ display: fullScreen ? 'none' : undefined }}>
<Box sx={{ display: hideFooter ? 'none' : undefined }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When not in modal, i.e. typing in a table filter, hide the footers so can see more of the results

@@ -46,7 +44,7 @@ export const StocktakeLineEditModal: FC<
okButton={
<DialogButton variant="ok" onClick={onOk} disabled={!isValid} />
}
height={height}
height={600}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer need the useKeyboardHeightAdjustment! So move all the heights back to hard coded

@@ -68,11 +68,10 @@ export const ReportArgumentsModal: FC<ReportArgumentsModalProps> = ({
}}
/>
}
contentProps={{ sx: { margin: '0 auto' } }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some styling is a little off in full screen mode, i need to review this more fully

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Must Have The product will not work without this refactor Team Korimako LachΓ©, Aimee, Noel John, and Zachariah
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve/simplify modal heights
1 participant