Skip to content

NamedNodeMap & Attr #59

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

Merged
merged 17 commits into from
Oct 29, 2023
Merged

NamedNodeMap & Attr #59

merged 17 commits into from
Oct 29, 2023

Conversation

garyb
Copy link
Member

@garyb garyb commented Sep 24, 2023

Originally by @flip111, #48

@flip111
Copy link
Contributor

flip111 commented Nov 10, 2024

@garyb i found another bug in your code

https://github.com/purescript-web/purescript-web-dom/blob/master/src/Web/DOM/DOMTokenList.js#L85-L93

TypeError: list.tokens is not iterable

The code i had before worked and i tested that it worked https://github.com/flip111/purescript-web-dom/blob/00114fabcf336a6b193fec77b4b8233dee2e591f/src/Web/DOM/DOMTokenList.js#L69-L80

to be honest i'm not happy that you closed my original pull request without comments / review. And that you took it upon yourself to make changes and stuff starts breaking

@garyb
Copy link
Member Author

garyb commented Nov 20, 2024

I don't exactly remember now, but I imagine I was trying to being helpful by fixing conflicts with main, making eslint happy, and changing a bunch of stuff to make the code idiomatic instead of sending it back to you to do so. 🤷‍♂️ Also integrating the *Name stuff at the same time rather than doing another pass of that. I probably had limited time and thought if I just do one pass over it at least everything will be sorted rather than it sitting for months with no response from me again.

You're right that I should have tested it though.

@flip111
Copy link
Contributor

flip111 commented Nov 20, 2024

I probably had limited time and thought if I just do one pass over it

Depends if you optimize your time for one big pass, or multiple smaller passes. In the latter case i could have done all that and you would just have to review it.

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