Enable user to configure modal's scrollBehavior by jmestxr · Pull Request #2355 · MarkBind/markbind

jmestxr · 2023-08-16T16:17:36Z

What is the purpose of this pull request?

Overview of changes:
User can now configure scroll position of modal by setting scroll-behavior prop to be 'inside' (default) or 'outside', e.g. <modal scroll-behavior='outside'> ...

The modal CSS styles for the respective scroll-behavior values are set within the opened() method which is called after modal is opened (see @opened event here).

For scroll-behavior = 'outside', basically what I did was:

Set the overflow-y property of the outermost modal container to scroll, which allowed user to mouseclick on scrollbar without causing modal to close unexpectedly. However, it resulted in this:

where the grey area is the part that user can click to close the modal and the white area is the overflow part.

To solve the above issue, I used javascript to query the modal content's total height (including the overflow part) to adjust the height of the grey part to fill up the white area, resulting in this:

Then adjust the background colors of relevant containers to get the following:

Screen.Recording.2023-08-16.at.11.37.44.PM.mov

Anything you'd like to highlight/discuss:

When both scroll-behavior='outside' and center='true' are set, modal does not render properly when content overflows. To fix this, I modified the code such that center='true' only takes effect when scroll-behavior='inside'.

Testing instructions:

cd into packages/cli/test/functional/test_site and run markbind serve -d
navigate to test_site/testModals.html in browser and test out the modals

Proposed commit message: (wrap lines at 72 characters)

Checklist: ☑️

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

Previously, modal is rendered properly when both scroll-behavior='outside' and center='true' is set. Remove effect of centering modal when scroll-behavior is set to 'outside'. center='true' has effect only if scroll-behavior is set to 'inside'

codecov · 2023-08-17T13:35:01Z

Codecov Report

Merging #2355 (79ad9a1) into master (cd5ba3d) will increase coverage by 0.28%.
Report is 2 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2355      +/-   ##
==========================================
+ Coverage   44.82%   45.11%   +0.28%     
==========================================
  Files         122      123       +1     
  Lines        5133     5165      +32     
  Branches     1082     1086       +4     
==========================================
+ Hits         2301     2330      +29     
- Misses       2513     2516       +3     
  Partials      319      319

Files Changed	Coverage Δ
packages/vue-components/src/Modal.vue	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

damithc · 2023-08-22T07:34:52Z

@jmestxr is this PR ready for review?

jmestxr · 2023-08-22T09:56:14Z

@jmestxr is this PR ready for review?

@damithc hi prof, this PR is ready for review

damithc · 2023-08-22T16:00:23Z

@MarkBind/active-devs for your review ...

tlylt · 2023-08-23T12:15:19Z

packages/vue-components/src/Modal.vue

+      // To adjust modal styles according to scrollBehavior
+
+      const modal = $vfm.get(this.id)[0].$el;
+      const modalContainer = modal.querySelector('.vfm__container');


does the properties mentioned here help you in the stuff you are doing below?

Ah, I can use those to refactor some of the styles to the <style> sheet, together with data() props. But I still need to use javascript to set overflow-y to the outermost container, and query the overflowed content height to adjust the modal container height.

I have just committed those changes

For outermost container, does using <vue-final-modal> class="abc" work?

It works. Thanks!

tlylt · 2023-08-23T12:19:04Z

Wondering whether bootstrap Modal'smodal-dialog-scrollable will be helpful here? https://getbootstrap.com/docs/5.1/components/modal/#scrolling-long-content

jmestxr · 2023-08-24T11:14:38Z

Wondering whether bootstrap Modal'smodal-dialog-scrollable will be helpful here? https://getbootstrap.com/docs/5.1/components/modal/#scrolling-long-content

Ah, this actually solves a previous PR #2322 in a more straightforward manner

I'll revert the solution from #2322 and use modal-dialog-scrollable instead for scroll-behavior='inside'

tlylt · 2023-08-26T07:01:39Z

@jmestxr
Followed your testing step here:

Testing instructions:
cd into packages/cli/test/functional/test_site and run markbind serve -d
navigate to test_site/testModals.html in browser and test out the modals

When clicking on the outside scrollbar, i'm getting the same old error of auto closing the entire modal. Could you verify/re-check?

Here's the gif:

In fact I just checked the preview of UG here

Encountering this exact error as well.

jmestxr · 2023-08-27T13:48:22Z

@jmestxr Followed your testing step here:

Testing instructions:
cd into packages/cli/test/functional/test_site and run markbind serve -d
navigate to test_site/testModals.html in browser and test out the modals

When clicking on the outside scrollbar, i'm getting the same old error of auto closing the entire modal. Could you verify/re-check?

Here's the gif:

In fact I just checked the preview of UG here

Encountering this exact error as well.

Apologies! I have just done a fix, notably for the calculation of isContentOverflow. The behavior should be correct now

tlylt · 2023-08-27T14:09:58Z

Apologies! I have just done a fix, notably for the calculation of isContentOverflow. The behavior should be correct now

Just tried it, seems very janky?

sometimes it just doesn't work, when i click on the scrollbar it closes the entire modal
if i resize the window, it behaviors weirdly

jmestxr · 2023-08-27T17:31:59Z

Apologies! I have just done a fix, notably for the calculation of isContentOverflow. The behavior should be correct now

Just tried it, seems very janky?

sometimes it just doesn't work, when i click on the scrollbar it closes the entire modal

if i resize the window, it behaviors weirdly

Okay, perhaps dynamically changing the height isn't the best way to go. Solution will become too unnecessarily complex just for this issue. Either way it doesn't solve the actual problem that the scroll-bar is within the closable area of the modal. Perhaps should instead try migrating vue-final-modal to higher versions which doesn't seem to have this issue: https://vue-final-modal.org/use-cases/modal-long-scroll

I think I'll close this PR and meanwhile open a new issue for solving #2322 using modal-dialog-scrollable?

tlylt · 2023-08-27T22:58:09Z

Okay, perhaps dynamically changing the height isn't the best way to go. Solution will become too unnecessarily complex just for this issue. Either way it doesn't solve the actual problem that the scroll-bar is within the closable area of the modal. Perhaps should instead try migrating vue-final-modal to higher versions which doesn't seem to have this issue: vue-final-modal.org/use-cases/modal-long-scroll

Agreed. Good exploration still 👍 I think the solution that I would go with is to either wait till we migrate vue-final-modal, or do a deep dive into the package, and find out what changes did they make to fix this issue in newer versions (might take time and have complicated issues, so I'm not recommending).

I think I'll close this PR and meanwhile open a new issue for solving #2322 using modal-dialog-scrollable?

Sure, if it turns out that using modal-dialog-scrollable can help significantly improve the implementation over the existing one then please go ahead. If not we can keep it as is.

jmestxr · 2023-08-28T01:45:23Z

Okay, perhaps dynamically changing the height isn't the best way to go. Solution will become too unnecessarily complex just for this issue. Either way it doesn't solve the actual problem that the scroll-bar is within the closable area of the modal. Perhaps should instead try migrating vue-final-modal to higher versions which doesn't seem to have this issue: vue-final-modal.org/use-cases/modal-long-scroll

Agreed. Good exploration still 👍 I think the solution that I would go with is to either wait till we migrate vue-final-modal, or do a deep dive into the package, and find out what changes did they make to fix this issue in newer versions (might take time and have complicated issues, so I'm not recommending).

I think I'll close this PR and meanwhile open a new issue for solving #2322 using modal-dialog-scrollable?

Sure, if it turns out that using modal-dialog-scrollable can help significantly improve the implementation over the existing one then please go ahead. If not we can keep it as is.

Will do. Thanks for the review @tlylt !

jmestxr added 4 commits August 16, 2023 23:49

Enable user to configure modal's scrollBehavior

b38f5b3

Merge branch 'master' into configure-modal-scrollbar-position

c2ea14e

Fix centering of modal

86830d3

Previously, modal is rendered properly when both scroll-behavior='outside' and center='true' is set. Remove effect of centering modal when scroll-behavior is set to 'outside'. center='true' has effect only if scroll-behavior is set to 'inside'

Update tests for Modal

b47d2fd

jmestxr marked this pull request as ready for review August 17, 2023 06:27

Add user documentation for scroll-behavior

614dbcf

tlylt reviewed Aug 23, 2023

View reviewed changes

Update test_site content with modal tests

9ba3ef4

jmestxr marked this pull request as draft August 24, 2023 04:07

Refactor styles into stylesheet

7479e65

Revert PR MarkBind#2322 with bootstrap's .modal-dialog-scrollable

eacf640

jmestxr marked this pull request as ready for review August 24, 2023 11:44

jmestxr added 2 commits August 24, 2023 22:39

Refactor style to :class in vue-final-modal

9ae58b6

Merge with master and resolve merge conflict

52d792a

jmestxr marked this pull request as draft August 24, 2023 14:58

jmestxr marked this pull request as ready for review August 24, 2023 14:58

Resolve merge conflict with master

28f555d

jmestxr added 2 commits August 27, 2023 21:37

Revise method to calculate isContentOverflow

684d4b5

Fix unwanted behavior of inner container's scrollbar appearing

79ad9a1

jmestxr closed this Aug 28, 2023

Conversation

jmestxr commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

damithc commented Aug 22, 2023

Uh oh!

jmestxr commented Aug 22, 2023

Uh oh!

damithc commented Aug 22, 2023

Uh oh!

tlylt Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

jmestxr Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

tlylt Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmestxr Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

tlylt commented Aug 23, 2023

Uh oh!

jmestxr commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlylt commented Aug 26, 2023

Uh oh!

jmestxr commented Aug 27, 2023

Uh oh!

tlylt commented Aug 27, 2023

Uh oh!

jmestxr commented Aug 27, 2023

Uh oh!

tlylt commented Aug 27, 2023

Uh oh!

jmestxr commented Aug 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmestxr commented Aug 16, 2023 •

edited

Loading

codecov bot commented Aug 17, 2023 •

edited

Loading

tlylt Aug 24, 2023 •

edited

Loading

jmestxr commented Aug 24, 2023 •

edited

Loading