Skip to content

Fix: modal content hidden#6508

Merged
PeerRich merged 4 commits intomainfrom
fix-modal-height
Jan 16, 2023
Merged

Fix: modal content hidden#6508
PeerRich merged 4 commits intomainfrom
fix-modal-height

Conversation

@G3root
Copy link
Copy Markdown
Contributor

@G3root G3root commented Jan 16, 2023

What does this PR do?

before:
Screenshot 2023-01-16 19:57:35

after:
https://www.loom.com/share/1d69040b06e944f1824e4ddbb5ce205c

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Test A
  • Test B

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't performed a self-review of my own code and corrected any misspellings
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my PR needs changes to the documentation
  • I haven't checked if my changes generate no new warnings
  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@G3root G3root requested a review from PeerRich January 16, 2023 14:29
@vercel
Copy link
Copy Markdown

vercel bot commented Jan 16, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
cal ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 16, 2023 at 3:36PM (UTC)

@G3root G3root requested a review from a team January 16, 2023 14:29
@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Jan 16, 2023

we need to thoroughly test this because I feel like we touched this file so many times and it always regressed somewhere else

@emrysal
Copy link
Copy Markdown
Contributor

emrysal commented Jan 16, 2023 •
edited
Loading

image

image

Of course this is an edge case, but let's make sure the overflow works for all dialogs (even for insanely vertically excessive viewports)

Tested Mobile & Desktop larger dialogs, looking good so far

@G3root
Copy link
Copy Markdown
Contributor Author

G3root commented Jan 16, 2023

@emrysal great findings, something i'm gonna look into my future PR's ❤️. pushed some changes to fix those edge cases.
Screenshot 2023-01-16 21:03:54
Screenshot 2023-01-16 21:05:21

emrysal
emrysal approved these changes Jan 16, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Agree with the change, I don't believe allowScroll would ever have a need to be set to false.

@PeerRich PeerRich merged commit 8243ac3 into main Jan 16, 2023
@PeerRich PeerRich deleted the fix-modal-height branch January 16, 2023 17:08
@Udit-takkar
Copy link
Copy Markdown
Contributor

Udit-takkar commented Jan 18, 2023

Screenshot 2023-01-18 at 11 02 35 PM

@emrysal @G3root allowScroll prop is required to prevent this from happening

@Udit-takkar
Copy link
Copy Markdown
Contributor

Udit-takkar commented Jan 18, 2023 •
edited
Loading

still unable to fix this react-select dropdown without allowScroll . tried few things from here JedWatson/react-select#4088 , JedWatson/react-select#4150

@G3root
Copy link
Copy Markdown
Contributor Author

G3root commented Jan 19, 2023

@Udit-takkar sorry my bad, I should've checked that. i can do a follow up reintroducing the allowScroll prop .

@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Jan 19, 2023

@Udit-takkar sorry my bad, I should've checked that. i can do a follow up reintroducing the allowScroll prop .

@G3root should we open a new PR before we merge to production today?

@Udit-takkar
Copy link
Copy Markdown
Contributor

Udit-takkar commented Jan 19, 2023

@G3root no issue it's more of a react-select bug on a dialog.

@G3root G3root mentioned this pull request Jan 20, 2023
Merged
7 tasks
fritterhoff pushed a commit to hm-edu/cal.com that referenced this pull request Jan 27, 2023
@G3root @fritterhoff
Fix: modal content hidden (calcom#6508)
8aedd77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emrysal emrysal emrysal approved these changes

@PeerRich PeerRich Awaiting requested review from PeerRich

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@G3root @PeerRich @emrysal @Udit-takkar