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

Change order onShow and insert node #116

Closed
wants to merge 1 commit into from

Conversation

grrodre
Copy link

@grrodre grrodre commented Feb 26, 2024

I came across the same problem as described here.

Getting the element in onShow callback to show/activate a modal won't work if doInsert is placed after the callback.

Changing the order seems to work.

Getting the element in onShow callback to show/activate a modal won't work if doInsert is placed after the callback.

This commit changes the order.
@sergei-maertens
Copy link
Collaborator

Hi - just letting you know I've seen this PR and hope to soon (within the next few weeks) have time to look into it!

@sergei-maertens
Copy link
Collaborator

sergei-maertens commented May 2, 2024

@grrodre in #119 I am working on solving the bundler/npm i challenge of this JS, I will probably incorporate your fix in that PR or otherwise handle it after that is merged, since the source files for the cookiebar module move.

But since the e2e tests seem to be passing, I suspect this is/will be a no brainer!

edit: it seems that the tests aren't actually running, I'll need to sort that out!

sergei-maertens added a commit that referenced this pull request May 2, 2024
The onShow handler should be invoked after the node was inserted.

Thanks to @grrodre for contributing the patch!
sergei-maertens added a commit that referenced this pull request May 2, 2024
The onShow handler should be invoked after the node was inserted.

Thanks to @grrodre for contributing the patch!
sergei-maertens added a commit that referenced this pull request May 3, 2024
The onShow handler should be invoked after the node was inserted.

Thanks to @grrodre for contributing the patch!
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.

2 participants