Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add MutationObserver for Blacklight 8 Modal #249

Merged
merged 1 commit into from
Dec 4, 2023
Merged

add MutationObserver for Blacklight 8 Modal #249

merged 1 commit into from
Dec 4, 2023

Conversation

yetti
Copy link
Contributor

@yetti yetti commented Dec 1, 2023

Blacklight 8 uses an HTML Dialog element instead of a Bootstrap modal, so it won't fire the 'shown.bs.modal' events when opened. This adds a MutationObserver to check if the 'open' attribute is added to the HTML Dialog element, to determine if it's open.

Blacklight 8 uses an HTML Dialog element instead of a Bootstrap modal, so it won't
fire the 'shown.bs.modal' events when opened. This adds a MutationObserver to
check if the 'open' attribute is added to the HTML Dialog element, to determine if
it's open.
@jcoyne
Copy link
Member

jcoyne commented Dec 1, 2023

Would it be simpler to have Blacklight itself emit an event that we can bind to here?

@yetti
Copy link
Contributor Author

yetti commented Dec 1, 2023

If we have Blacklight Modal emit its own events, doesn't that bring us part of the way back to the functionality that Bootstrap modals provided? I'm not across the reasons for moving to HTML Dialog in the first place, but it was definitely a loss of extensibility.

In any case, my reasoning for using a Mutation Observer was to not rely on a third party extension point again, but with what the HTML element provides natively (the "open" attribute).

@jrochkind
Copy link
Member

If BL were to emit it's own event, then this func makes a future b-l-r rely on some future version of Blacklight (not work with current ones), as well as requiring PR's to both places.

If b-l-r just uses native functionality, there is less compatibility confusion, and a future b-l-r can work with current Blacklight versions.

On the other hand, of course the choice to use mutation observer means everyone that wants to observe needs to re-implement this behavior... but it's not that hard to implement is it? (Or is it?) I share yetti's assumption that BL's choice to use native modal was meant to keep things simple and tell clients to use native functionality; complex logic that changes from version to version of BL has been problematic in the past.

@jcoyne jcoyne merged commit 3de6a19 into projectblacklight:main Dec 4, 2023
6 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants