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): Add minCharacters prop #1690

Merged
merged 2 commits into from
May 22, 2017

Conversation

dyesseyumba
Copy link
Contributor

@dyesseyumba dyesseyumba commented May 21, 2017

I've added minCharacters props in Dropdown as requested in #1124. Now we can set the minimum characters for a search to begin showing results.

I also would like to add saveRemoteData but I'm wondered if it's not useless. Because someone will use Flux' store and someone else the Redux' store and the other the locale storage.

So should I use locale storage to implement saveRemoteData?

@codecov-io
Copy link

codecov-io commented May 21, 2017

Codecov Report

Merging #1690 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1690      +/-   ##
==========================================
- Coverage   99.75%   99.75%   -0.01%     
==========================================
  Files         141      141              
  Lines        2412     2411       -1     
==========================================
- Hits         2406     2405       -1     
  Misses          6        6
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 100% <100%> (ø) ⬆️
src/collections/Table/TableFooter.js 100% <0%> (ø) ⬆️

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 bacf4b7...254fc19. Read the comment docs.

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

Changes look great. Just a couple code style comments for addressing please.

@@ -714,13 +718,15 @@ export default class Dropdown extends Component {

if (onSearchChange) onSearchChange(e, newQuery)

// open search dropdown on search query
if (search && newQuery && !open) this.open()
if (newQuery.length >= this.props.minCharacters) {
Copy link
Member

Choose a reason for hiding this comment

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

In keeping with the code style, please destructure minCharacters on line 715.

@@ -1604,6 +1604,19 @@ describe('Dropdown', () => {
dropdownMenuIsOpen()
})

it('Don\'t open the menu on change if query\'s length is less than minCharacters', () => {
Copy link
Member

Choose a reason for hiding this comment

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

When a string includes single quote characters, please quote it with double quotes to prevent escaping.

@levithomason
Copy link
Member

Regarding saveRemoteData, I agree completely. I do not think this prop is applicable in React apps. I think we just skip this prop entirely. The best practice today is to use some data flow solution, such as Redux. Almost all of these solutions have some form of interoperability with localStorage.

@levithomason
Copy link
Member

Thanks much, will merge on pass.

@dyesseyumba
Copy link
Contributor Author

You're welcome

@levithomason levithomason merged commit 46e1bfe into Semantic-Org:master May 22, 2017
@layershifter layershifter changed the title feat(Dropdown): Add minCharacters props. Fixes #1124 feat(Dropdown): Add minCharacters prop May 22, 2017
@levithomason
Copy link
Member

Released in semantic-ui-react@0.68.4.

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.

None yet

3 participants