-
Notifications
You must be signed in to change notification settings - Fork 81
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
watch for [data-inert] elements and turn them inerted as well (#126) #128
base: main
Are you sure you want to change the base?
Conversation
@robdodson @alice please have a look at the PR I created. I also added some test, that I copied from the basic tests. |
Hey @babielmam, quick question: does this actually set |
No, it doesn't set the |
Sorry for being so slow to get to this. The code mostly looks good, but I'd like a couple more tests as suggested in my comment. |
@alice , @robdodson I reworked the parts mentioned in my last comment. I also added some more tests. Please take a look :) |
I think Alice is out of office till the 13th, but I'll see if she and I can catch up on this then. |
Just looking at it, it looks pretty good I, think. It seems just a little unfortunate to me that this has to be a thing, but it seems like a reasonable request and actual contribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary to set data-inert
, since folks who prefer to use the data attribute can use el.dataset.inert
.
test/specs/data-attribute.spec.js
Outdated
@@ -0,0 +1,29 @@ | |||
describe('Data-Attribute', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add some tests for mutations?
- Adding an element into an inert subtree
- Adding data-inert to an existing element
- Adding an element to the DOM with a data-inert attribute already on it
This allows to use the data-inert attribute to allow the polyfill and have valid HTML syntax. Related to #126