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

[WIP, don't merge] Fix #1804 and tweak #1595 #1865

Merged
merged 10 commits into from
Jun 1, 2018
Merged

Conversation

pygy
Copy link
Member

@pygy pygy commented May 30, 2017

This fixes #1804 and tweaks #1595.

removeAttr() now mirrors setAttr(). values set to undefined or null are equivalent to a lack of value, except for select, where null

There are still possibly problematic corner cases with select.value, please don't merge this yet.

#1862 can be merged as is in the mean time if you want. I'll rebase this.

@dead-claudia
Copy link
Member

FYI: #1862 is now merged.

@dead-claudia
Copy link
Member

dead-claudia commented Aug 24, 2017

@pygy Ping...how close to ready is this to merge (another PR might be blocked on this)?

@porsager
Copy link
Contributor

@pygy did you want to include the changes for style as suggested here: #1867 in this pull request?

@dead-claudia
Copy link
Member

@pygy Would you mind redoing this PR since #1949 made several changes to event handling? (It should be more tenable to do now in that area.)

@pygy
Copy link
Member Author

pygy commented Apr 20, 2018

@vasilrimar reported issues where passing undefined doesn't remove the attr. I need to check if this patch covers his use case.

modified version of his repro

Copy link
Contributor

@tivac tivac left a comment

Choose a reason for hiding this comment

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

I can only give this a cursory look-through but it seems reasonable to me.

render/render.js Outdated
if(attrs.value === null) {
if (vnode.dom.selectedIndex !== -1) vnode.dom.value = null
} else {
/*eslint-disable no-implicit-coercion*/
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried // eslint-disable-next-line no-implicit-coercion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I went for // eslint-disable-line since it is what we use the most, and normalized the way we use those directives in the code base.

Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

Looks good to me. (I didn't want to abandon this patch.)

@pygy
Copy link
Member Author

pygy commented Jun 1, 2018

Thanks @isiahmeadows the review is most welcome :-)

@pygy pygy merged commit 3f5cabc into MithrilJS:next Jun 1, 2018
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.

Handlers are not removed while set to undefined on update
4 participants