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

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

Merged

Conversation

prosdev0107
Copy link
Contributor

@prosdev0107 prosdev0107 commented Feb 24, 2022

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.
Screenshot at Feb 24 07-56-43

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
Copy link

codecov bot commented Feb 24, 2022

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.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

prosdev0107 commented Feb 24, 2022

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
Copy link
Member

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
Copy link
Contributor Author

@michael-s-molina That sounds good.

@rusackas
Copy link
Member

rusackas commented Mar 3, 2022

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

@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 7, 2022
@prosdev0107
Copy link
Contributor Author

@michael-s-molina @evans .

Resolved the review.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

prosdev0107 commented Mar 8, 2022

@michael-s-molina

Resolved.

@michael-s-molina
Copy link
Member

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

@pull-request-size pull-request-size bot added size/S and removed size/M labels Mar 9, 2022
@rusackas
Copy link
Member

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

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

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

@michael-s-molina michael-s-molina merged commit e7355b9 into apache:master Mar 18, 2022
villebro pushed a commit that referenced this pull request Apr 3, 2022
…rom token separators… (#18926)

* make that the comma can be added by removing it from token separators in select component.

* fix(explore comma): add the allowTokenSeperators props into Select

* fix(explore comma): make to allow to customize the token separators in Select.

* fix(explore comma): make to add the unit test and story book.

* fix(explore comma): make to fix the lint

* fix(explore comma): make to fix the spell  & add tokenSeparatprs props to PickedSelectProps

* Update Select.tsx

* fix(explore comma): make to run lint fix

(cherry picked from commit e7355b9)
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/S 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants