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

Spread attributes to container. #123

Merged
merged 13 commits into from
Jan 23, 2016
Merged

Conversation

srph
Copy link
Contributor

@srph 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?

This would allow us to pass event listeners and all sort
of things. See gpbl#122.
@gpbl
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
@srph
Copy link
Contributor Author

srph commented Jan 23, 2016

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

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