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

Add search bar for filtering flags in home page #268

Merged
merged 2 commits into from
Jun 20, 2019

Conversation

yosyad
Copy link
Contributor

@yosyad yosyad commented Jun 4, 2019

Description

I added a search bar that can help to find a flag throw a big number of flags on the home page.

Motivation and Context

When the usage of flagr grows, I tend to add more flags for every configuration I have. By doing that, I affect directly the length of the list of flags on the home page. I was trying to suggest a new way to navigate throw all the flags easily.

How Has This Been Tested?

Tested locally, manually.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov-io
Copy link

codecov-io commented Jun 4, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #268   +/-   ##
=======================================
  Coverage   81.76%   81.76%           
=======================================
  Files          26       26           
  Lines        1530     1530           
=======================================
  Hits         1251     1251           
  Misses        210      210           
  Partials       69       69

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 43030e8...58e4867. Read the comment docs.

@zhouzhuojie
Copy link
Collaborator

Hey, this is GREAT!

I've wanted to do something like the search for a while, but didn't decide which way to go, client side or server side search for the UI?

The advantage of server side search is that we get the limit/offset pagination upfront to be scalable into huge amount of flags. And client side search for its simplicity.

@yosyad
Copy link
Contributor Author

yosyad commented Jun 7, 2019

I don't know if flagr home page is a good case for pagination and server side filtering, since I cant see a use case of too many flags (more than 1000?).
I guess that there are a lot of ways we can manage huge amount of flags, for example by grouping them or other approach. In my opinion this is a good start that we can continue with, maybe if will be a future requests from other users, we could implement a more sophisticated solution, such as server side filtering or even pagination. Please share your thoughts.

this.filteredFlags = this.flags.filter(
({ id, description}) =>
id.toString().startsWith(this.searchTerm) ||
description.startsWith(this.searchTerm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to make it a substring search instead of just startsWith? At least, the search should be as good as cmd+f from the browser. This is equivalent to "description_like" params, so we can easily migrate the search box to server-side search in the future (if it's necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zhouzhuojie Alright, done.

@zhouzhuojie zhouzhuojie merged commit 6dd6c33 into openflagr:master Jun 20, 2019
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.

3 participants