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

Fix modal resizing #498

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix modal resizing #498

wants to merge 1 commit into from

Conversation

mjaydenkim
Copy link
Contributor

Summary

When adding a lot of majors or resizing text window, modal is now scrollable with hidden scrollbar.

PR Type

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Mobile + Desktop Screenshots & Recordings

fix-modal-resizing.mp4

QA - Test Plan

Tested on variety of screen sizes/modes, appears to function similarly on mobile and work more effectively on desktop.

Breaking Changes & Notes

Major options now appear from the top like the grades. They would previously spill off the modal screen, which is kinda ugly and also problematic when scrolling is involved.

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ notion
  • πŸ• ...
  • πŸ“• ...
  • πŸ™… no documentation needed

What GIF represents this PR?

gif

Also added ability to specify whether or not options should appear from top or bottom in select dropdowns (now, major menu appears from the top so it doesn't spill off the window).
@mjaydenkim mjaydenkim requested a review from a team as a code owner January 14, 2025 00:43
@dti-github-bot
Copy link
Member

[diff-counting] Significant lines: 27.

@mjaydenkim
Copy link
Contributor Author

does anyone have insight why this test is failing? not really sure what it means

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

Successfully merging this pull request may close these issues.

2 participants