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

docs(props): fix uniqueness #1780

Merged
merged 3 commits into from
Jun 22, 2017
Merged

docs(props): fix uniqueness #1780

merged 3 commits into from
Jun 22, 2017

Conversation

layershifter
Copy link
Member

Before

_899
_900

After

_899
_901

Why?

SUI.WIDTHS contains both numbers and strings (example: 1 and '1'), but it does not matter for the docs. Also, as we use value as key and React converts key to string, we have duplicate, but invisible elements.

const evalValue = value => _.uniq(eval(value)) // eslint-disable-line no-eval
const evalValue = value => eval(value) // eslint-disable-line no-eval

const uniqValues = values => _.uniqWith(values, (val, other) => val == other) // eslint-disable-line eqeqeq
Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot use there strict equals, because 1 and '1' aren't equal.

Copy link
Member

Choose a reason for hiding this comment

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

Coerce them both to numbers with the + operator:

+val === +other

Example:

val = '5'

typeof val   // => "string"
typeof +val  // => "number"
val === 5    // => false
+val === 5   // => true

Copy link
Member Author

@layershifter layershifter Jun 19, 2017

Choose a reason for hiding this comment

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

Yes, but not all items of array are numbers:

val = 'five'
+val // => NaN

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can do following:

`${val}` === `${other}`
// or
val.toString() === other.toString()

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, that works as well.

@codecov-io
Copy link

codecov-io commented Jun 19, 2017

Codecov Report

Merging #1780 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1780   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files         142      142           
  Lines        2459     2459           
=======================================
  Hits         2453     2453           
  Misses          6        6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04ea612...daf2e58. Read the comment docs.

@layershifter layershifter changed the title docs(Props): fix uniqueness docs(props): fix uniqueness Jun 19, 2017
Copy link
Member Author

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

I've performed requested change, LGTM

@levithomason levithomason merged commit 9d88060 into master Jun 22, 2017
@levithomason levithomason deleted the docs/fix-uniq branch June 22, 2017 01:00
@levithomason
Copy link
Member

Thank ya!

@levithomason
Copy link
Member

Released in semantic-ui-react@0.69.0

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