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

Add method getElementsByClassName for Element #100

Closed

Conversation

dungmv
Copy link

@dungmv dungmv commented Aug 13, 2020

I made a pr to add method getElementsByClassName for Element
#47

@dungmv dungmv changed the title Add get elements by class name for element Add method getElementsByClassName for Element Aug 13, 2020
Copy link
Member

@brodybits brodybits left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I think tests are needed.

Another thing is that the mutation report shows that the code that is moved has not been tested very well: https://dashboard.stryker-mutator.io/reports/github.com/brodybits/xmldom/master#dom.js

@dungmv
Copy link
Author

dungmv commented Aug 17, 2020

Thanks for the contribution. I think tests are needed.

Another thing is that the mutation report shows that the code that is moved has not been tested very well: https://dashboard.stryker-mutator.io/reports/github.com/brodybits/xmldom/master#dom.js

I don't use it before, Please explain to me more detail about what I need to do?

Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

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

It would be much easier to understand the difference when the location of the methods would not have changed, are you able to move the two methods back to line 597?

Edit: Just understood that it was moved to a different prototype, so just ignore this comment.

@brodybits
Copy link
Member

We are using Stryker for mutation testing to check the quality of the code coverage. It is documented here: https://stryker-mutator.io/

@karfau
Copy link
Member

karfau commented Jan 21, 2021

Just to clarify:

@dungmv I think those additions are very nice to have.
We have some jest tests now and if you are familiar with that, it would really be nice to at least test the things you added.

If you don't know how to do it it's also fine, we can help or take over, just let us know here.

@dungmv
Copy link
Author

dungmv commented Jan 21, 2021

@karfau Sorry, I don't know how to do it, so can you take over it?

@karfau karfau added the help-wanted External contributions welcome label Jan 21, 2021
@karfau karfau added this to the 0.6.0 milestone Jan 21, 2021
@karfau karfau added the documentation Improvements or additions to documentation label Jan 21, 2021
@karfau karfau self-assigned this Jan 24, 2021
@karfau
Copy link
Member

karfau commented Jan 24, 2021

I just started looking into this.
First thing I now understood is that those method are already existing on the master branch as part of the Document prototype and this PR moves them to the Element prototype.
But the implementation has also been modified along the way.
Then I checked the specs again to see if there are any differences between the methods on Document and Element. I found out that not even the Living standard specifies getElementById on the Element interface. So I don't think we should invest time to add it. specially since it's always available from every Element this way: element.ownerDocument.getElementById(id).

When I revert that change, this PR is "adding" getElementsByClassName to the Element interface which, as mentioned before is only present on the "Living Standard" so it's much lower priority for xmldom right now, just before the next release of some critical fixes (from my perspective).

I'll continue to work on this at some point in time (if nobody else does).

@karfau karfau removed the spec:DOM-Level-2 https://www.w3.org/TR/DOM-Level-2-Core/ label Jan 24, 2021
since no standard defines it on the `Element` interface
and it's always reachable as `element.ownerDocument.getElementById`

xmldom#100 (comment)
@shunkica
Copy link
Collaborator

This doesn't look right. According to the spec the method should return a HTMLCollection

@karfau
Copy link
Member

karfau commented Jul 22, 2023

@shunkica the page you linked to also says that the HTMLCollection is a deprecated interface.
I think the LiveNodeList is our equivalent/closest implementation, even though it is missing the name based accessor, so I think that part is not the main issue in this PR.

Edit: To summarize the thread of this PR: The only thing missing for this to land is adding tests for the method on both the Document and the Element level.

@shunkica
Copy link
Collaborator

I am having trouble seeing where you found that HTMLCollection is deprecated.
If you are talking about the historical artifact part, that doesn't necessarily mean deprecated.
Also, like you've pointed out, LiveNodeList doesn't include namedItem.
From my POV this PR currently only introduces one method (or actually, just moves it) and it doesn't fully conform to the spec.

@karfau
Copy link
Member

karfau commented Jul 22, 2023

In general I think every small improvement is great, not every touched code has to be 100% aligned to a spec in order to be merged.

We have to go step by step, whenever somebody has capacity for something.
Otherwise we end up with many huge conflicting PRs because every PR tries to solve all the issues that are discovered along the way.

Of course that's just my perspective, I'm totally open for more actively contributing maintainers with different opinions and happy to discuss our options.

But I just discovered something:
This PR is so outdated, that it moves a totally broken implementation of that method (hence the conflicts), so I will close it.

@karfau karfau closed this Jul 22, 2023
@karfau karfau removed help-wanted External contributions welcome documentation Improvements or additions to documentation labels Jul 22, 2023
@karfau karfau removed this from the before 1.0.0 milestone Jul 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:DOM Living Standard https://dom.spec.whatwg.org/ testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getElementsByClassName() for Element
5 participants