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 unexpected closing of longer modal when clicking on scrollbar #2322

Merged
merged 5 commits into from
Jul 15, 2023

Conversation

WillCWX
Copy link
Contributor

@WillCWX WillCWX commented Jul 6, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Fixes #2013
Fixes this of #1916 :

When the modal is triggered, the scroll bar appears to come about from the center of the screen and shifts immediately to the right, a little weird to see that.

Overview of changes:

  • Moves scroll bar to div class="modal-body".
  • Other CSS adjusted to keep the gap between the dialog window and the browse window
  • Updates modal tests accordingly.

Anything you'd like to highlight/discuss:

The max-height of the div class="modal-content" is set to 90vh to enable scrolling as max-height is infinite otherwise.
90vh looks best to me but I don't have very good eyes for design so do play around with the number to see what maybe best.

I have not tested scrolling in the x-axis as I have yet to find a modal that is longer than the webpage but adding max-width=90vw may fix this easily as well.

Testing instructions:

Add this topic1.md file to a markbind website.
The modal should have a scrollbar that is clickable and looks like this:

image
the white bar is from snip and not from the website

Proposed commit message: (wrap lines at 72 characters)
Fix unexpected closing of longer modal when clicking on scrollbar

Fix modal scroll

Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Thanks for the work @WillCWX! Some minor comments :)

packages/vue-components/src/Modal.vue Outdated Show resolved Hide resolved
packages/vue-components/src/Modal.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the work @WillCWX

@tlylt
Copy link
Contributor

tlylt commented Jul 12, 2023

Hi @WillCWX, wondering if you have explored ways to make it work without moving the scrollbar into the modal? Would be great if we could provide both options, perhaps something like what Chakra UI offers:

image

https://chakra-ui.com/docs/components/modal/usage#modal-overflow-behavior

@tlylt tlylt self-requested a review July 15, 2023 12:28
@tlylt
Copy link
Contributor

tlylt commented Jul 15, 2023

Personally I think its better to have the max height as 100vh to be consistent with previous display

@yucheng11122017 what do you mean by previous display? Existing modal without this PR change has a "padding" at the bottom, which is achievable if we do with max height = 100% + some other CSS. I think this might be more comfortable visually hence let's go with this instead 😺.
image

I adjusted the style slightly to

  • achieve the margin around the dialog window
  • move the scrollbar into the modal body instead of having it scroll from the header
  • and also updated the test case.

This is not the best solution out there but I think it does address the issue for now. In fact, scrolling within the body seems to be the solution for long modals for v2 of vue-final modal (slight difference in that they have more margin) :
image

Will post a separate issue to track modal improvements.

Some investigation note:

@tlylt tlylt added this to the v5.0.0 milestone Jul 15, 2023
@tlylt tlylt changed the title Fix modal scroll Fix unexpected closing of longer modal when clicking on scrollbar Jul 15, 2023
@tlylt tlylt merged commit 222c290 into MarkBind:master Jul 15, 2023
5 checks passed
@tlylt
Copy link
Contributor

tlylt commented Jul 15, 2023

@all-contributors please add @WillCWX for code

@allcontributors
Copy link
Contributor

@tlylt

I've put up a pull request to add @WillCWX! 🎉

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.

Longer Modal closes unexpectedly when interacting with the scrollbar
3 participants