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

Spread attributes to container. #123

Merged
merged 13 commits into from Jan 23, 2016

Conversation

2 participants
@srph
Contributor

srph commented Jan 21, 2016

This would allow us to pass event listeners and all sort
of things. Closes #122.

Do not merge yet, needs documentation.

edit: oops, right before that — should we remove the explicit style props?

srph added some commits Jan 21, 2016

Spread attributes to container.
This would allow us to pass event listeners and all sort
of things. See #122.
@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jan 22, 2016

Owner

Thanks @srph, also for adding a test 🙌

Yes, I believe that style, onKeyDown and tabIndex should be treated as standard attributes.

  • tabIndex should have a default value as 0 (it is already like that), it would eventually be set by the developer to -1 when not needed
  • onKeyDown requires special care as in wrote in this comment

If you prefer, I can push these changes. For the docs, don't worry, I'll update them as the PR is merged.

Owner

gpbl commented Jan 22, 2016

Thanks @srph, also for adding a test 🙌

Yes, I believe that style, onKeyDown and tabIndex should be treated as standard attributes.

  • tabIndex should have a default value as 0 (it is already like that), it would eventually be set by the developer to -1 when not needed
  • onKeyDown requires special care as in wrote in this comment

If you prefer, I can push these changes. For the docs, don't worry, I'll update them as the PR is merged.

@srph

This comment has been minimized.

Show comment
Hide comment
@srph

srph Jan 22, 2016

Contributor

I made some updates. I think I'm quite lost (primarily with the tests) -- but I can revert stuff back to the original (before these new commits today)). Or I can just clean up the mess first, and ping you here in GH in a few hours.

(writing code during Fridays is bad; I never learn haha)

For tabIndex, currently it is set as such: canChangeMonth && tabIndex. I'm not sure if I understand the intent. Can I ask why it's written that way? I think it should be canChangeMonth ? tabIndex : -1 (assuming that it is used to disable focus).

If it looks good, I can squash the commits first, though.

Edit: I wrote some test cases for the new behavior of handleKeyDown. But, I'll commit them later.

Contributor

srph commented Jan 22, 2016

I made some updates. I think I'm quite lost (primarily with the tests) -- but I can revert stuff back to the original (before these new commits today)). Or I can just clean up the mess first, and ping you here in GH in a few hours.

(writing code during Fridays is bad; I never learn haha)

For tabIndex, currently it is set as such: canChangeMonth && tabIndex. I'm not sure if I understand the intent. Can I ask why it's written that way? I think it should be canChangeMonth ? tabIndex : -1 (assuming that it is used to disable focus).

If it looks good, I can squash the commits first, though.

Edit: I wrote some test cases for the new behavior of handleKeyDown. But, I'll commit them later.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jan 22, 2016

Owner

Looks fine to me thanks! (Don't worry about the decreased coverage, i'll check it out)
Thinking twice I see the tabIndex will work properly as it is. It should be added only if months can be changed, but its value may came from an external prop.

Owner

gpbl commented Jan 22, 2016

Looks fine to me thanks! (Don't worry about the decreased coverage, i'll check it out)
Thinking twice I see the tabIndex will work properly as it is. It should be added only if months can be changed, but its value may came from an external prop.

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jan 22, 2016

Owner

Let me know if you need helps with the test... I know it's a bit too messy there, I should separate them in different files.

Owner

gpbl commented Jan 22, 2016

Let me know if you need helps with the test... I know it's a bit too messy there, I should separate them in different files.

@srph

This comment has been minimized.

Show comment
Hide comment
@srph

srph Jan 23, 2016

Contributor

Hmm, anything I missed out?

By the way, how about the role props/attribute? Should we allow it to be overridable?

Contributor

srph commented Jan 23, 2016

Hmm, anything I missed out?

By the way, how about the role props/attribute? Should we allow it to be overridable?

@gpbl

This comment has been minimized.

Show comment
Hide comment
@gpbl

gpbl Jan 23, 2016

Owner

Nothing, it's perfect! 👍 Going to cut a new release soon 🚀

Owner

gpbl commented Jan 23, 2016

Nothing, it's perfect! 👍 Going to cut a new release soon 🚀

gpbl added a commit that referenced this pull request Jan 23, 2016

@gpbl gpbl merged commit 6fa63af into gpbl:master Jan 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@srph

This comment has been minimized.

Show comment
Hide comment
@srph

srph Jan 23, 2016

Contributor

Alright, I hope I didn't break anything crosses fingers. Thanks for your work!

Contributor

srph commented Jan 23, 2016

Alright, I hope I didn't break anything crosses fingers. Thanks for your work!

This was referenced Jan 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment