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

Select VNode - add support selectedIndex property #1425

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

anthony-redFox
Copy link
Contributor

Before submitting a PR please:

  • Include tests for the functionality you are adding! See CONTRIBUTING.md for details how to run tests.
  • Make sure you have based your branch off the dev branch.
  • Submit your PR against the dev branch.
  • Run npm run build and check that the build succeeds.
  • Ensure that the PR hasn't been submitted before.

PR Template

Objective

This PR...

Closes Issue

It closes Issue #...

@anthony-redFox
Copy link
Contributor Author

I am working on unit tests

@coveralls
Copy link
Collaborator

coveralls commented Dec 10, 2018

Coverage Status

Coverage increased (+0.007%) to 94.872% when pulling 0a31eb8 on anthony-redFox:master into cf3776b on infernojs:master.

@anthony-redFox
Copy link
Contributor Author

Done!

@anthony-redFox
Copy link
Contributor Author

@Havunen Please, take a look


it('Should strict render select if value set', () => {
render(
<select selectedIndex={3} value={'3'}>
Copy link
Member

Choose a reason for hiding this comment

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

What about a case where selectedIndex is different than value? Who wins?

Copy link
Member

Choose a reason for hiding this comment

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

can you add test case with expected result so its thought use case then

@Havunen
Copy link
Member

Havunen commented Dec 13, 2018

Did you check if selectedIndex need to be added to typings?

Copy link
Member

@Havunen Havunen left a comment

Choose a reason for hiding this comment

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

Can you add the test & check typescript

@Havunen
Copy link
Member

Havunen commented Jan 21, 2019

Lets merge this @anthony-redFox , Thanks for PR! I will do some testing and small cleaning!

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

Successfully merging this pull request may close these issues.

3 participants