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

#3899 - Small Screen Responsiveness for Modal - Student #4227

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

sh16011993
Copy link
Collaborator

@sh16011993 sh16011993 commented Jan 13, 2025

As a part of this PR, the following were completed:

  • Removed the min-width dialog property from the Base Modal Dialog since it wasn't getting used anywhere in the codebase. Having it was causing the issue i.e. not allowing the dialog to be flex when the screen size was reduced.
    Issue:
image
  • Removed the TODO: remove mx-auto in stable version of vuetify to center modelDialog from the Base Modal Dialog
  • Added the code logic to allow the Base Modal Dialog to behave like a full screen dialog for smaller screen devices like the mobile and the tablets.

Demo video attached:

2025-01-14.14-37-10.mp4

@sh16011993 sh16011993 self-assigned this Jan 13, 2025
@sh16011993 sh16011993 added Student Student Features Web portal labels Jan 13, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice way to achieve the conditional modal view ❤️
Would the below not achieve the same?
Please disregard the 500 although it produced a nice result during my tests also.

const mediaQuerySmallToLarge = window.matchMedia("(max-width: 500px)");
function handleScreenChangeSmallToLarge(e: MediaQueryList) {
  console.log(e);
  showFullScreen.value = e.matches;
}
mediaQuerySmallToLarge.addEventListener("change", () =>
  handleScreenChangeSmallToLarge(mediaQuerySmallToLarge),
);
handleScreenChangeSmallToLarge(mediaQuerySmallToLarge);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note that (max-width: 500px), (max-height: 300px) can also be used to check with or height. Doing this we may need to add/remove the with="auto" also, so, when the modal expanded is used the with="auto" probably need to be removed. Not a blocker since it may be not part of the ticket.

Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Great work, please take a look at the comments.

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.49% ( 3850 / 17122 )
Methods: 10.37% ( 224 / 2161 )
Lines: 25.88% ( 3322 / 12837 )
Branches: 14.31% ( 304 / 2124 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.59% ( 589 / 898 )
Methods: 59.63% ( 65 / 109 )
Lines: 68.72% ( 468 / 681 )
Branches: 51.85% ( 56 / 108 )

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 86.14% ( 1249 / 1450 )
Methods: 82.42% ( 136 / 165 )
Lines: 88.51% ( 1032 / 1166 )
Branches: 68.07% ( 81 / 119 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 67.77% ( 5992 / 8842 )
Methods: 65.54% ( 738 / 1126 )
Lines: 71.62% ( 4695 / 6555 )
Branches: 48.15% ( 559 / 1161 )

@lewischen-aot lewischen-aot self-requested a review January 14, 2025 23:12
Copy link
Collaborator

@lewischen-aot lewischen-aot left a comment

Choose a reason for hiding this comment

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

LGTM, as the result matches the AC. Good job 👍

@sh16011993 sh16011993 added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 1d42c16 Jan 15, 2025
21 checks passed
@sh16011993 sh16011993 deleted the 3899_small_screen_responsiveness_for_modal_student branch January 15, 2025 00:13
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Student Student Features Web portal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants