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

Take boolean attributes into account #49

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Conversation

ryota-ka
Copy link
Contributor

Updated modules/props.js so that it will take boolean attributes into account.
(false values for such attributes shouldn't be interpreted as "false")

modules/props.js Outdated Show resolved Hide resolved
@acstll
Copy link
Member

acstll commented Dec 20, 2019

Thank you for the PR.

Do you think you could add a test for this change?

@ryota-ka
Copy link
Contributor Author

@acstll Thank you for your quick response.

I've added a test case for this change.
Also, I've changed const to var in 00c5476.

If it looks fine, please let me know before merging. I'll squash my commits.

@acstll
Copy link
Member

acstll commented Dec 20, 2019

Looks good to me!

I'll ping @paldepind to double check before releasing this. I saw he was active on this project recently, and I haven't touched this in a very long time.

This is a bug fix but I'd do a major (6.0.0) since I believe this could break people's code, no?

@ryota-ka
Copy link
Contributor Author

I've squashed the fixup! commit and force-pushed.

@paldepind Please feel free to merge this if you feel it's okay 🙏

@ryota-ka ryota-ka requested a review from paldepind January 22, 2020 05:50
@ryota-ka
Copy link
Contributor Author

@paldepind Pinging for your review (Sorry for interruption 🙇)

@ryota-ka
Copy link
Contributor Author

@acstll It's been four months.
Shall we wait for @paldepind's review any more?

Personally I'm also 👍 for making a major version bump.

@acstll
Copy link
Member

acstll commented Apr 13, 2020

Hey, time flies. Sorry you had to wait this long. I think we can move forward. I'll merge and cut a major release this week.

@acstll acstll merged commit 14df9da into snabbdom:master Apr 13, 2020
@acstll
Copy link
Member

acstll commented Apr 13, 2020

It's done @ryota-ka I hope everything's OK, haven't done this in a while! Thanks again for your patience.

@ryota-ka
Copy link
Contributor Author

@acstll I also appreciate your review and releasing the new version! I'll enjoy 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