Skip to content

Remove modals with duplicate id#2012

Merged
jonahtanjz merged 3 commits intoMarkBind:masterfrom
jonahtanjz:fix-duplicate-modals
Aug 18, 2022
Merged

Remove modals with duplicate id#2012
jonahtanjz merged 3 commits intoMarkBind:masterfrom
jonahtanjz:fix-duplicate-modals

Conversation

@jonahtanjz
Copy link
Contributor

@jonahtanjz jonahtanjz commented Aug 17, 2022

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:

Overview of changes:
Fixes #2008
Fixes #1909

The issue is caused by having modals of the same id on the same page (usually because the modals are included more than once using <include>). Removing modals with ids that already exist fixes the issue. The issue with duplicated modals opening is also resolved.

Anything you'd like to highlight/discuss:
Previous implementation allows for modals with the same id to be included/present on the page more than once. Having just 1 copy of the modal still allows the included <trigger>s to open the modal. Not sure if I missed out on any edge cases where more than 1 modal of the same id needs to be on the page.

Testing instructions:

  1. Create multiple <modal>s and <trigger>s with the same id
  2. Serve the site
  3. All triggers should still work but only 1 modal shows up

Proposed commit message: (wrap lines at 72 characters)
Remove modals with duplicate id

Modals with duplicate id on the same page will result in
triggers opening multiple modals and can cause the page
to be unscrollable after the modal is closed.

Let's remove modals with duplicate id so that only one
copy of the modal for each id will exist on the page.


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

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

Thanks @jonahtanjz for the fix, since it also resolves the modal issue #1909 on our UG, perhaps we can link it in the PR description to close it as well.

I was thinking about this problem for a bit, and I wonder whether there are other cases of this problem due to how include is implemented, which may cause the id collision issue. For example, anchor scroll handling and scroll spy for pageNav. Will try to look into these in the coming weeks...

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

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

LGTM on my end

@tlylt tlylt added this to the 4.0.1 milestone Aug 17, 2022
@jonahtanjz jonahtanjz merged commit 4683ca6 into MarkBind:master Aug 18, 2022
@damithc
Copy link
Contributor

damithc commented Aug 18, 2022

@jonahtanjz Thanks for fixing this. Perhaps do a release, so that I can start using the bug fix in production sites?

@jonahtanjz
Copy link
Contributor Author

Thanks for fixing this. Perhaps do a release, so that I can start using the bug fix in production sites?

Just did a release :)

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.

The page becomes unscrollable after opening and closing modal Modal not working correctly in UG Page Navigation Menus section

3 participants