Initial implementation of SearchFilter

[ttannis]

Summary of Changes

This PR created a SearchFilter component and subcomponents, and introduces some component modifications in preparation for introducing filtering. However the component will not yet be exposed -- the UI will remain the same when this is merged. Changes include:

SearchPanel

1. Modified SearchPanel to be a stateless functional component, as it's purpose is to layout its children accordingly. It now takes any number of children and lays them out in a column.

SearchPage

1. Modified the use of SearchPanel on SearchPage according to the changes made to SearchPanel.

SearchFilter

1. Created a re-useable CheckBoxItem that takes checkbox type input props and utilizes props.children for customizing what is rendered in the label.
2. Created a SearchFilter component. It currently takes checkBoxFilterSections -- an array of objects containing the json information to render the given UI -- and onFilterChange -- to be executed when a checkbox is toggled. The work in this PR only lays out the component. TODOs have been left in the code in places where we will have to complete functionality. The UI when this is exposed:
   

Tests

Added tests for new components and updated tests for existing components according to how they've been modified.

Documentation

N/A. This PR does not add any new features that need to be documented.

CheckList

Make sure you have checked all steps below to ensure a timely review.

• PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
• PR includes a summary of changes, including screenshots of any UI changes.
• PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public python functions and the classes in the PR contain docstrings that explain what it does
• PR passes all tests documented in the developer guide
• I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message"

[codecov-io]

Codecov Report

Merging #260 into feature/search_v2 will increase coverage by 0.18%.
The diff coverage is 96.96%.

@@                  Coverage Diff                  @@
##           feature/search_v2     #260      +/-   ##
=====================================================
+ Coverage              70.83%   71.01%   +0.18%     
=====================================================
  Files                    128      130       +2     
  Lines                   3329     3354      +25     
  Branches                 411      412       +1     
=====================================================
+ Hits                    2358     2382      +24     
- Misses                   939      940       +1     
  Partials                  32       32

Impacted Files	Coverage Δ
...lication/static/js/components/SearchPage/index.tsx	98.4% <100%> (+0.01%)	⬆️
...s/components/SearchPage/ResourceSelector/index.tsx	95.23% <100%> (ø)	⬆️
...js/components/common/Inputs/CheckBoxItem/index.tsx	100% <100%> (ø)
...tic/js/components/SearchPage/SearchPanel/index.tsx	100% <100%> (ø)	⬆️
...ic/js/components/SearchPage/SearchFilter/index.tsx	95.23% <95.23%> (ø)

---

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 03ceb3b...43633aa. Read the comment docs.

[ttannis]

Will delete references to SearchFilter before merging.

[danwom]

nit: could be renamed SearchFilterCheckbox.

Probably out of scope but we could also define a generic input type and extend it to define checkboxes, radio buttons, and text inputs. Then we could use something like inputProperties: InputConfig[] if we need to show multiple input types.

[ttannis]

I named this based on thinking of the potential UI extensions that would include inputs that aren't checkboxes, it the component is named for it's purpose but it just so happens that all we have now are checkboxes. Up for revisiting naming if we ever get around to extending this and decide whether or not want to break down components further.

[danwom]

nit: SearchFilterInput[] vs Array<SearchFilterInput>

[danwom]

Might be worthwhile to abstract away the actual click event since most users of a checkbox are not interested in the event itself.

<input onChange={ (e) => this.props.onChange(e.target.value) }

[ttannis]

Unless we have reason to, this pattern is something that is discouraged due the "jsx-no-lambda rule". https://jonhilton.net/typescript-and-react-forbidden-lambdas/

My understanding is that this is not a "bad" thing to do, but rather that they should be used more thoughtfully and perhaps only when needed. For example I know I've used it in OwnerEditor because of a scoping problem. Do you have any strong feelings or thoughts on this before this is merged?

[danwom]

I used the lambda for convenience. I should have written out the full example as below.

https://github.com/lyft/amundsenfrontendlibrary/blob/c9ba2b878fbc7e10d5e991d63324a14c62291d7c/amundsen_application/static/js/components/SearchPage/ResourceSelector/index.tsx#L35-L49

Maybe this works differently for React.SFC since you can't define class functions. Don't have very strong feelings about this, but I think we should aim to make reusable components as simple to use as possible.

[ttannis]

I see. I think one major factor here does involve the fact that this is a React.SFC as you pointed out. The properties given will be placed directly on the input element, and TypeScript requires that this method match the expected web api.

Beyond that, I also see the purpose of this kind of component to be creating something that we can reuse anywhere in our application for current and future use cases. Therefore I believe we should design first for all use cases and second for ease of use -- but also weigh the use case coverage against how much more difficult it makes the component to use. In this case, I think designing for all use cases is preferable since it doesn't add any burden beyond having to grab event.target.value instead of value. In terms of "all use cases", one case that comes to mind is the one where we might want info from event.currentTarget and/or event.target.

However I think the concept you raised is always worth keeping in mind as we create these components -- if it were the case that we created something that would require doing some kind of pre-processing (example: something that needs to sanitize the data in some way before using it), then that common component should get upgraded to a React.Component and have some helper method in the way that you described.