Skip to content

Conversation

@hengliyin
Copy link
Contributor

Add the clock scale.Use showNumber prop to show scale.
wx20180410-163515

<Clock
     size={200}
     value={value}
     showNumber
/>

src/entry.js Outdated
import Clock from './Clock';

import './Clock.css';
import './Clock.less';
Copy link
Owner

Choose a reason for hiding this comment

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

That can't be like that - less is compiled to css on build for compatibility reasons. Not everybody uses less.

src/Mark.jsx Outdated
}}
/>

{number ?
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of ternary, you can simply use this syntax: argument && <Component>

src/Clock.jsx Outdated
width={hourMarksWidth}
/>,
);
if (this.props.showNumber) {
Copy link
Owner

Choose a reason for hiding this comment

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

This could be a ternary. No need to duplicate this much code.

<Mark
  ...
  number={this.props.showNumber ? i : null}
  ...
/>

@wojtekmaj
Copy link
Owner

That's super nice! Thanks!

@hengliyin
Copy link
Contributor Author

Thanks for your advice. I'll change it and continue to submit it.

@eddit
Copy link

eddit commented May 1, 2018

Any update on this PR? Excited about the new feature.

@wojtekmaj wojtekmaj merged commit d1eb065 into wojtekmaj:master May 4, 2018
@wojtekmaj
Copy link
Owner

Sorry, been super busy lately. @yhlben thanks for contributing and for all the fixes, perfect!

@wojtekmaj wojtekmaj self-assigned this May 4, 2018
@wojtekmaj wojtekmaj added this to the 2.3.0 milestone May 4, 2018
@eddit
Copy link

eddit commented May 4, 2018

Good deal, thanks man. Are you still pushing updates to the npm package?

@wojtekmaj
Copy link
Owner

Yes, absolutely. 2.3.0 was released with this change, although I changed flag name to renderNumbers to match other props used in the component :)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants