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

Extend stars component #36

Merged
merged 30 commits into from
Jan 3, 2017
Merged

Extend stars component #36

merged 30 commits into from
Jan 3, 2017

Conversation

kaytcat
Copy link
Contributor

@kaytcat kaytcat commented Jan 2, 2017

  • Allow stars to be rateable.
  • Also accessible and scalable.

@feychou
Copy link
Contributor

feychou commented Jan 2, 2017

I really like how we extended Stars with just one one additional prop, it still looks p clean so no need for composition after all!

I do not really understand how multiColors works, maybe we can make it more straightforward.

}

export default class Stars extends Component {
static propTypes = {
color: PropTypes.string,
multiColors: PropTypes.array,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have fixed values for these arrays (color and multiColors) so that we can use check for the values of the elements in the array? Alternatively, can we use a more strict validation with PropTypes.arrayOf? This would make this issue easier to solve in the future #6

multiColors: PropTypes.array,
name: PropTypes.string,
selectable: PropTypes.bool,
totalStars: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

We could define the biggest value here, it could maybe be 10?

@@ -0,0 +1 @@
<Stars value={2} selectable multiColors={['red', 'orange', 'yellow', 'green', 'blue']}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Props should be added on different lines in the docs too.

@feychou
Copy link
Contributor

feychou commented Jan 3, 2017

What is missing in the API is a something like onUpdate that would let the host application define a method to be called every time the value of the stars changes.

@feychou
Copy link
Contributor

feychou commented Jan 3, 2017

Also when everything is ready the docs should be built again - sigh.

@feychou feychou merged commit 916bd5d into master Jan 3, 2017
@feychou feychou deleted the KUNPRO-67-implement-star-selector branch January 3, 2017 14:09
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