fix(native-filters): Fix native filters config modal

[simcha90]

SUMMARY

This PR fix next flows:

1. There was regression, when you mark filter as Required it doesn't shows errors under filter and also not marks it with red (also FilterBar also FiltersConfigModal)

2. When user switch between filter types with Default Value some times it crashes dependent on what filter were changed

3. For filter without Column field it doesn't load default value

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

Merging #15506 (08b6c47) into master (f62cea3) will increase coverage by 0.00%.
The diff coverage is 77.08%.

❗ Current head 08b6c47 differs from pull request most recent head d9051e6. Consider uploading reports for the commit d9051e6 to get more accurate results

@@           Coverage Diff           @@
##           master   #15506   +/-   ##
=======================================
  Coverage   76.95%   76.96%           
=======================================
  Files         976      976           
  Lines       51283    51296   +13     
  Branches     6901     6911   +10     
=======================================
+ Hits        39467    39480   +13     
  Misses      11597    11597           
  Partials      219      219           

Flag	Coverage Δ
javascript	71.39% <77.08%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ashboard/components/gridComponents/ChartHolder.jsx	68.68% <ø> (ø)
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	20.83% <0.00%> (-4.17%)	⬇️
...filters/components/GroupBy/GroupByFilterPlugin.tsx	0.00% <0.00%> (ø)
...s/components/TimeColumn/TimeColumnFilterPlugin.tsx	0.00% <0.00%> (ø)
...ers/components/TimeGrain/TimeGrainFilterPlugin.tsx	0.00% <0.00%> (ø)
...c/filters/components/Select/SelectFilterPlugin.tsx	80.70% <82.35%> (+0.52%)	⬆️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	73.65% <92.85%> (+0.54%)	⬆️
...veFilters/FilterBar/FilterControls/FilterValue.tsx	67.28% <100.00%> (+1.60%)	⬆️
...src/filters/components/Range/RangeFilterPlugin.tsx	90.76% <100.00%> (-0.14%)	⬇️
superset-frontend/src/filters/components/common.ts	100.00% <100.00%> (ø)
... and 1 more

---

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 f62cea3...d9051e6. Read the comment docs.

[amitmiran137]

look good
just a small question before merging

[amitmiran137]

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true

[github-actions]

@amitmiran137 Ephemeral environment spinning up at http://52.25.166.59:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[amitmiran137]

@simcha90 is that behavior valid?

on range filter with default value with the full range it doesn't considered to be a default value

[simcha90]

@simcha90 is that behavior valid?

on range filter with default value with the full range it doesn't considered to be a default value

yes it's valid as discussed with @villebro it means as unset value, it also behavior when you clicks Clear all

[amitmiran137]

LGTM!

tested out all filter types with required and column scenarios and same flow is always the same on every type

• if no column chosen no default value to choose
• if required and default value also checked
• if required but column then only if save validation message of both column and default value appears like :
  Fill all required fields to enable "Default Value"
  Default value is required

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[michael-s-molina]

The removal of "Value is required" was not a regression. It was a design decision to eliminate the unnecessary spacing among filters. That's why we added the asterisk. This PR reintroduced the text and the spacing.

@simcha90 Can you please roll back to the previous behavior?

@villebro @junlincc @amitmiran137

[junlincc]

@amitmiran137 @simcha90 Hi friends! according to the thread, we are seeing some regressions from this PR. To prevent this from happening Please do leave enough time for community reviews, and we would do the same :). One thing that could help the project is all parties to sync what feature goals we each wanna achieve on a weekly basis.

We try to stabilize this native filter for prime time, and are investing more resources into QA, we would love to help keeping the PR standard high by testing more, please simply leave us 48 for code reviews and testing.
cc @rosemarie-chiu @graceguo-supercat

[michael-s-molina]

Another regression found: the default value Select is not checked when there's a default value.

screen-recording-2021-07-06-at-15844-pm_hwyyB5LM.mp4

[junlincc]

@simcha90 let's make sure we have both behavior reverted. many thanks 🙏

[simcha90]

Hi @junlincc @michael-s-molina yep, you are right it's good point to wait for some more feedbacks before merge, I will take it in account for next PRs

About notes, because of this PR fix some other behavioral bugs, I'd prefer create one new PR to fix your notes rather then revert this one, to fix your notes I have some questions:

1. About Spacing / Coloring:

We have here 2 features in one:

a. Adding red colors to filters dependent on the type of filter: like border for Select Filter or make Range Filter is red for required state - this feature is not affecting layout, only change color of the filters

(see example here: #15506 (comment))

b. Adding Value is required text and spacing between items - this feature affecting layout and add spacing, so we have 2 ways here:

• Revert this text for all filters as you proposed in first
• Add this text only when needed and in other cases just remove this space

2. Bug with initial value I'll create separate PR for it

@junlincc @michael-s-molina can you please give your input for first item? And I will create PR ASAP to fix it, thanks

[michael-s-molina]

1- About Spacing / Coloring:

a. Keep as it is with the red colors.

b. Remove the text only in the filter bar, not in the modal. The asterisk and the color are sufficient. We don't want the flick effect that happens when we add the text dynamically.

Thank you.

[simcha90]

@michael-s-molina @junlincc
Hi, I created PR according your proposals: #15572
Can you please review it and give your input? Thanks...

[piewaer]

Hello, I started front end engineering in the superset-frontend folder, but get lots of errors.

1. In the superset-frontend folder, npm ci
2. npm run dev-server
3. 98% of the process, get lots of errors, includes the type error of the data and the error not found by the module

It should be unable to parse JSX, But how do I need to solve it?Looking forward to your reply