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

feat(Dropdown): make deburr opt-in, and deburr input #2223

Merged

Conversation

patrikmolsson
Copy link
Contributor

@patrikmolsson patrikmolsson commented Oct 20, 2017

WIP pull request. Would gladly appreciate feedback on both tests and source code!

Fixes #2163.

  • Make deburr opt-in
  • When having deburr, the input string should also be deburred.
  • Add proper documentation
  • Add example of using deburr
  • Add example of custom search function
  • Add proper "should not filter diacritics when deburr is not set" test

@patrikmolsson patrikmolsson changed the title fix(Dropdown): make deburr opt-in, and deburr input feat(Dropdown): make deburr opt-in, and deburr input Oct 21, 2017
@codecov-io
Copy link

codecov-io commented Oct 21, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@4f29636). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2223   +/-   ##
=========================================
  Coverage          ?   99.73%           
=========================================
  Files             ?      151           
  Lines             ?     2626           
  Branches          ?        0           
=========================================
  Hits              ?     2619           
  Misses            ?        7           
  Partials          ?        0
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 100% <100%> (ø)

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 4f29636...74e18d6. Read the comment docs.

@patrikmolsson
Copy link
Contributor Author

@levithomason I think I'm done now, but I would love some input on the prop naming before we merge!

@@ -0,0 +1,25 @@
import React from 'react'
import _ from 'lodash'
import { Dropdown } from 'semantic-ui-react'
Copy link
Member

Choose a reason for hiding this comment

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

Let's sort imports in alphabetical order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course! 😄 On it!

Is there a linting rule for this that I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

Nope, we don't have or don't use it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. Btw, how do you sort it? Alphabetically by module name, i.e. react, lodash, and semantic-ui-react?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--> lodash, react, semantic-ui-react

Copy link
Member

Choose a reason for hiding this comment

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

Yes, by module name.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for updating. FWIW, there is a fixable lint rule for this that we should turn on sort-imports so this is done automatically for people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@levithomason It seems that lint does not sort on the module name, but rather on the imported member, or am I missing something?

Copy link
Member

@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.

@patrikmolsson Thanks, awesome work 👍 I've left styling comments, let's fix them

search={caseSensitiveSearch}
selection
fluid
placeholder={'Try to search for case or CASE'}
Copy link
Member

Choose a reason for hiding this comment

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

And props in alphatical order, too 😄

search
selection
deburr
options={options}
Copy link
Member

Choose a reason for hiding this comment

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

^

@@ -787,7 +791,7 @@ export default class Dropdown extends Component {
// There are times when we need to calculate the options based on a value
// that hasn't yet been persisted to state.
getMenuOptions = (value = this.state.value, options = this.props.options) => {
const { multiple, search, allowAdditions, additionPosition, additionLabel } = this.props
const { multiple, search, allowAdditions, additionPosition, additionLabel, deburr } = this.props
Copy link
Member

Choose a reason for hiding this comment

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

Let's sort props there too

@patrikmolsson
Copy link
Contributor Author

patrikmolsson commented Oct 23, 2017

@layershifter There it is!

By the way, what are your thoughts on the prop naming? Is it clear what deburr means?

@layershifter
Copy link
Member

@patrickgaskill fuzzy looks better to me, however lodash uses deburr. It's a complicated question

@patrikmolsson
Copy link
Contributor Author

@layershifter fuzzy might also imply that searching for ac will match abc...

@patrikmolsson
Copy link
Contributor Author

I've thought about this, but cannot really find an appropriate third option.

@levithomason
Copy link
Member

Beautiful PR, I can see the care and attention to detail you put in. ❤️

Prop name

Regarding the prop name, lodash is the most installed NPM package and it calls this process deburring. It is likely many people will recognize that term. However, there is a formal specification and process for this.

A Unicode character with diacritics is called a "precomposed character". Example, é is composed of the letter e and ´ (acute accent). When these characters are separated like this they are "decomposed" (see previous link).

In terms of Unicode equivalence, this is called "normalization". Java has a class called "Normalizer" which performs this operation. The Unicode Standard includes a Normalization Chart which maps these characters to one another. The technical process by which equivalent Unicode values are determined is called decomposition.

So, I think we could go with deburr, normalize, or decompose. I have my opinion, but I'll let you two choose.


@layershifter this is good to merge once you guys are settled on the most fitting prop name.

@layershifter
Copy link
Member

I'm think that we can go with deburr.

@patrikmolsson
Copy link
Contributor Author

@levithomason Thanks for the input, and for the kind words! As @layershifter says, let's go with deburr ☺️

@patrickgaskill
Copy link
Contributor

@layershifter You tagged the wrong Patrick ;)

@layershifter
Copy link
Member

@levithomason Can you merge this? As codecov failed, I don't have enough rights 😄

@levithomason levithomason merged commit 64bf4a1 into Semantic-Org:master Nov 4, 2017
@levithomason
Copy link
Member

Sorry folks, Codecov is having issues finding a base to compare some PRs against and incorrectly failing statuses.

This will go out in the next release, today or tomorrow.

@levithomason
Copy link
Member

Released in semantic-ui-react@0.76.0

@patrikmolsson patrikmolsson deleted the feature/deburr-opt-in branch November 5, 2017 13:48
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.

5 participants