Skip to content

Conversation

@brianteeman
Copy link
Contributor

This allows us to have auto-dismiss with and without the manual dismiss x

Thanks @C-Lodder

This allows us to have auto-dismiss with and without the manual dismiss x

Thanks @C-Lodder

/* Lifecycle, element appended to the DOM */
connectedCallback() {
this.setAttribute('role', 'alertdialog');
Copy link
Contributor

@wilsonge wilsonge Sep 17, 2019

Choose a reason for hiding this comment

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

Are we deliberately setting this back to hardcoded role="alertdialog"? (i.e. reverting #119) - you said in #118 this was the incorrect solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my mistake - should not have changesd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copy/pasted @C-Lodder code after testing it for autodismiss. Didnt spot these other changes. I guess he edited an older version of the file. I will put them back and retest before updating this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK should be correct now. I revert the dialog changes and retested and all seems good.

@brianteeman
Copy link
Contributor Author

I "think" this might fix the error we are getting on the main repo as well

10:11:18.724 SEVERE - http://localhost/test-install/media/vendor/joomla-custom-elements/js/joomla-alert.min.js?00d88ab1111327528710d8641fba25d9 0:1742 Uncaught TypeError: Cannot read property 'removeChild' of null

@wilsonge wilsonge merged commit 792cca2 into joomla-projects:master Oct 3, 2019
@brianteeman
Copy link
Contributor Author

Thanks

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