fix(explore comma): make that the comma can be added by removing it from token separators…

[prosdev0107]

SUMMARY

Cannot add comma to Number Formatting

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Comma doesn't show up, and the cursor jumps to the beginning of the line. Typing a period then erases everything.
It is because that the comma is used as the token separator in select component.
After:
I fixed this issue by removing the comma from the token separators.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• 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 #18926 (ed74275) into master (b5e9fad) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #18926      +/-   ##
==========================================
- Coverage   66.76%   66.73%   -0.03%     
==========================================
  Files        1670     1668       -2     
  Lines       64398    64278     -120     
  Branches     6499     6500       +1     
==========================================
- Hits        42993    42896      -97     
+ Misses      19722    19699      -23     
  Partials     1683     1683              

Flag	Coverage Δ
javascript	51.31% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx	37.25% <ø> (ø)
.../src/explore/components/controls/SelectControl.jsx	60.71% <ø> (ø)
superset-frontend/src/components/Select/Select.tsx	86.38% <100.00%> (+0.05%)	⬆️
superset/extensions.py	93.90% <0.00%> (-4.91%)	⬇️
superset/models/core.py	88.91% <0.00%> (-0.14%)	⬇️
superset/connectors/base/models.py	88.58% <0.00%> (-0.04%)	⬇️
superset/dashboards/schemas.py	99.41% <0.00%> (-0.01%)	⬇️
superset/viz.py	58.26% <0.00%> (ø)
superset/databases/api.py	93.99% <0.00%> (ø)
superset/models/sql_lab.py	91.55% <0.00%> (ø)
... and 14 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 b5e9fad...ed74275. Read the comment docs.

[geido]

Hello @prosdev0107 thanks for the contribution! However, I think we shouldn't take this path as the comma is a very common separator. I think we need to override the separators in those instances where it is required. As @michael-s-molina pointed out also, the decimal places separators also vary by locale. Let me know if you want to tackle this problem!

[prosdev0107]

Hi @geido .
Thank you for your review.

Okay, I agree .

@geido @michael-s-molina
Do you think that we can override the separators by adding the separators props into Select component?

Please leave the comment if you have a free time.

[michael-s-molina]

I think a more clean solution is to expose the tokenSeparators property that already exists in Ant Design instead of creating a new one.

type PickedSelectProps = Pick<
  AntdSelectAllProps,
  | 'allowClear'
  | 'autoFocus'
  | 'disabled'
  | 'filterOption'
  | 'labelInValue'
  | 'loading'
  | 'notFoundContent'
  | 'onChange'
  | 'onClear'
  | 'onFocus'
  | 'placeholder'
  | 'showSearch'
  | 'tokenSeparators' <-- include this
  | 'value'
>;

...

<StyledSelect
   ...
   tokenSeparators={tokenSeparators || TOKEN_SEPARATORS}
   ...
>

All default separators are valid for this control with the exception of the comma.

const y_axis_format: SharedControlConfig<'SelectControl'> = {
   ...
   tokenSeparators: ['\n', '\t', ';']
}

[prosdev0107]

@michael-s-molina That sounds good.

[rusackas]

If you do enable the AntD prop, it might be good to add a control to the interactive Storybook.

[prosdev0107]

@michael-s-molina @evans .

Resolved the review.

[michael-s-molina]

You don't need to create another property in SelectProps, you just need to add tokenSeparators in PickedSelectProps as you can see in my previous comment. You also misspelled tokenSeparators (you wrote tokenSepErators)

[prosdev0107]

@michael-s-molina

Resolved.

[michael-s-molina]

@prosdev0107 You can run npm run lint-fix to see/fix the formatting problems.

[rusackas]

@prosdev0107 just give this a quick rebase, and we should be good to go.

[michael-s-molina]

LGTM. Thanks for the fix.

[geido]

LGTM! @prosdev0107 you have a conflict to resolve. Thanks!