9396 form filter #584
9396 form filter #584
Conversation
Now it also dismisses on click as it should.
Seems like we should somehow be able to use the jquery module from rails, perhaps using a strategy similar to rails/webpacker#738, but it seems like installing via npm is necessary.
Add 'id' for a11y and only pass single default value
Needed for things like class variables.
Some test assertion within Enzyme seems to be misbehaving when given stub components; react-test-renderer never had a problem with this. This is a common issue others have, but I couldn't find any good solutions besides suppressing the warnings.
React lib seems to initialize on its own
cooperka
force-pushed
the
9396_form_filter
branch
from
cc5f66f to
cee887b
Compare
There was a problem hiding this comment.
Magnificent! Overall very clean and great.
Some tweaks to functionality and style:
I think the text in the box should read form:"Form B".
Can we please get a consistent padding on all sides of the popover.
Button styles are changing a bit as a result of the other issue I'm working on. Right now the search button and your button are a bit different but let's leave for now and plan to do a styling pass at the end.
Regarding if/when we merge this, perhaps we can do a kind of feature flag thing. Can you please make display of filters contingent on the presence (in Ruby) of Settings.filters_beta (so Settings.filters_beta.present?? Then I can set that env var on the staging server and devs (like you) can set it in their config/settings.local.yml. We will want to be able to demo this stuff progressively to client.
Great stuff!
| * Converts a list of forms from the backend into something Select2 understands. | ||
| */ | ||
| const parseFormsForSelect2 = (allForms) => allForms | ||
| .map((form) => mapKeys(form, (value, key) => key === "displayName" ? "text" : key)); |
There was a problem hiding this comment.
Why is this outside of the class? Seems like something that would be a private method. Is this how private methods are accomplished in React?
There was a problem hiding this comment.
This is effectively a private method (though JavaScript doesn't have that concept), but more importantly, it's static.
I could have instead declared it a static class method but that's messier to factor out. I like putting utils outside of the class for ease of future refactoring. As soon as some other component wants to use this logic, it can move into a shared utils.js or something.
| options={{ | ||
| placeholder: I18n.t("filter.chooseForm"), | ||
| dropdownCssClass: "filters-select2-dropdown", | ||
| width: "100%", |
There was a problem hiding this comment.
Yes for better git diff. The linter will soon enforce this too.
| render() { | ||
| return ( | ||
| <OverlayTrigger | ||
| containerPadding={25} |
There was a problem hiding this comment.
Seems wrong to have style details like this in here. Or am I misunderstanding what this is...
There was a problem hiding this comment.
I agree 😢 Bootstrap requires this. This adds a pseudo-margin to the overlay itself so it doesn't hit the side of the screen.
cooperka
force-pushed
the
9396_form_filter
branch
from
697432f to
0902764
Compare
To indicate what it's currently filtering by
Select2 wrapper doesn't work well with 'empty' options, which was the next best solution.
|
@smoyth all feedback is addressed |
This adds a new
Filterscomponent containing aFormFilterbutton.Filtersso they don't "play nice" together. That should be done before release as part of 9402 (Kevin).form:(foo|bar)text from the advanced searchbox. We should probably prioritize this separately from the rest of that issue.