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

watch for [data-inert] elements and turn them inerted as well (#126) #128

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

babielmam
Copy link

This allows to use the data-inert attribute to allow the polyfill and have valid HTML syntax. Related to #126

@babielmam
Copy link
Author

@robdodson @alice please have a look at the PR I created. I also added some test, that I copied from the basic tests.
I added a linebreak-style option to the ES-lint file because my IDE was otherwise reporting them as false. I think it shouldn't hurt to have it in there.

@robdodson
Copy link
Collaborator

Hey @babielmam, quick question: does this actually set data-inert on any of the elements?

@babielmam
Copy link
Author

babielmam commented Sep 5, 2019

No, it doesn't set the data-inert attribute.
As I looked over the code just right now, I see some cases where only inert is watched for but not data-inert. So, if you wish I can rework those parts

@alice
Copy link
Member

alice commented Sep 6, 2019

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.

@babielmam
Copy link
Author

babielmam commented Oct 4, 2019

@alice , @robdodson I reworked the parts mentioned in my last comment. I also added some more tests. Please take a look :)

@robdodson
Copy link
Collaborator

I think Alice is out of office till the 13th, but I'll see if she and I can catch up on this then.

@bkardell
Copy link
Collaborator

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.

Copy link
Member

@alice alice left a 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.

@@ -0,0 +1,29 @@
describe('Data-Attribute', function() {
Copy link
Member

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

@yoavweiss yoavweiss changed the base branch from master to main March 5, 2021 21:37
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.

5 participants