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

Add overflow-auto to modals to make them scrollable #754

Merged
merged 3 commits into from Dec 30, 2022

Conversation

bennymi
Copy link
Contributor

@bennymi bennymi commented Dec 30, 2022

Before submitting the PR:

  • Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
  • Did you update and run tests before submission using npm run test?
  • Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
  • Did you update documentation related to your new feature or changes?

What does your PR address?

Fixes #734.

Added overflow-auto to the modal to make it scrollable.

Tips

  • Tap "convert to draft" to indicate this is work in progress.
  • Link to an issue using the verbiage Fixes #XX
  • Linked issues will auto-close when the PR is merged.

@vercel
Copy link

vercel bot commented Dec 30, 2022

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

Name Status Preview Updated
skeleton-docs ✅ Ready (Inspect) Visit Preview Dec 30, 2022 at 10:34PM (UTC)

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 30, 2022

@bennymi Thanks for jumping on this.

Is there any way to keep a small bit of spacing between the modal and the edge of the screen? This still looks a bit broken:

Screen Shot 2022-12-30 at 3 27 57 PM

Perhaps adding some padding to the backdrop, which surrounds the modal window?

Screen Shot 2022-12-30 at 3 34 16 PM

@bennymi
Copy link
Contributor Author

bennymi commented Dec 30, 2022

@endigo9740 I think the best we can do is add a mt-4 to the modal div, which would look like this:

image

But this then means that when the whole modal is visible it isn't fully centered anymore. Although the small difference isn't that noticable:

image

Using my-4 on the modal div or p-4 on the backdrop div doesn't work.

@endigo9740
Copy link
Contributor

Gotcha, I figured that might be the case. Whatever works and looks good really.

Also side note - where is that drag handle coming from in the bottom-right corner. That's not native right? Do you have a browser plugin or something triggering that? First I've seen of that.

@bennymi
Copy link
Contributor Author

bennymi commented Dec 30, 2022

@endigo9740 yeah that's actually from the browser 😄 Firefox and Chrome both have that feature where you can change the dimensions of the screen, that's when the drag handle shows up:

image

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 30, 2022

Ohhh I see. I didn't realize that's what you were using. I tend to opt for the fixed sizes for mobile previews in Chrome as oppose to the fluid option. I follow.

Btw, it's still not quite like I want. Do you mind if I jump in an take a quick swing at this? I'll push a small change in a moment if I can get it like I want it. (Sorry I'm so picky heh)

@bennymi
Copy link
Contributor Author

bennymi commented Dec 30, 2022

@endigo9740 sure thing, give it a go! :)

@endigo9740
Copy link
Contributor

Got it! Also fixed the trigger buttons being jacked up on mobile as well. I'll push this on sec...

Screen Shot 2022-12-30 at 4 32 56 PM

Screen Shot 2022-12-30 at 4 32 34 PM

@endigo9740
Copy link
Contributor

endigo9740 commented Dec 30, 2022

I'm pleased with this, merging now!

@bennymi
Copy link
Contributor Author

bennymi commented Dec 30, 2022

@endigo9740 nice! Looks much better than before!

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.

Modal is not scrollable
2 participants