You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rabumaabraham (via GitHub)" <gi...@apache.org> on 2026/10/09 13:28:42 UTC

[PR] fix(column-config): allow commas in D3 number formats [superset]

rabumaabraham opened a new pull request, #45145:
URL: https://github.com/apache/superset/pull/45145

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   fix(column-config): allow commas in D3 number formats
   -->
   
   ### SUMMARY
   Add the ,.0f preset to the D3 number format options for column customization, displaying numbers with thousands separators and no decimal places (for example, 12,345).
   
   Explicitly configure token separators to use newlines, tabs, and semicolons instead of treating commas as separators. This allows users to enter comma-formatted D3 number formats without the input splitting the value.
   
   The preset is available in both D3 format and Small number format.
   
   Fixes #45136
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   #### Before
   <img width="1917" height="1020" alt="before superset" src="https://github.com/user-attachments/assets/5f6d89b3-1c50-459d-8193-781308e295b1" />
   
   #### After
   <img width="1917" height="1027" alt="after superset" src="https://github.com/user-attachments/assets/3c88421c-c172-49e5-bc84-247871ac1f71" />
   
   
   ### TESTING INSTRUCTIONS
   
   1. Open a dashboard or chart in Explore.
   2. Navigate to Customize โ†’ Customize columns.
   3. Open the D3 format dropdown and verify that ,.0f (12,345) is available.
   4. Repeat for Small number format.
   5. Enter ,.0f manually in either field and verify that the comma remains part of the format.
   6. Apply the configuration and verify that the displayed numbers use thousands separators and no decimal places.
   
   #### Automated checks:
   
   - npm --prefix superset-frontend test -- --watch=false ColumnConfigConstants.test.tsx
   - Oxlint on both changed files: 0 errors.
   - git diff --check: passed.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Required feature flags:
   - [x] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] 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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4234162958


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,23 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+
 const d3NumberFormat: ControlFormItemSpec<'Select'> = {
   allowNewOptions: true,
   controlType: 'Select',
   label: t('D3 format'),
   description: D3_FORMAT_DOCS,
-  options: D3_FORMAT_OPTIONS.map(option => ({
+  options: d3NumberFormatOptions.map(option => ({
     value: option[0],
     label: option[1],
   })),
   defaultValue: D3_FORMAT_OPTIONS[0][0],
   creatable: true,
+  tokenSeparators: ['\n', '\t', ';'],

Review Comment:
   Addressed. I added a comment explaining that commas are valid in D3 number formats and therefore must not be included in `tokenSeparators`. I also added a test to verify the expected token separators.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4230688601


##########
docker-compose.yml:
##########
@@ -107,7 +108,7 @@ services:
     image: postgres:17
     restart: unless-stopped
     ports:
-      - "127.0.0.1:${DATABASE_PORT:-5432}:5432"
+      - "127.0.0.1:${DATABASE_PORT:-5433}:5432"

Review Comment:
   **Suggestion:** With no `DATABASE_PORT`, Postgres is published on host port 5433, but the development guide directs users to localhost:5432, so their connections miss this database.
   
   **Assessment:** ๐ŸŸ  `Major` ยท ๐Ÿ” `Occurrence: Sometimes` ยท ๐Ÿท๏ธ `Api mismatch`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a06efb8c1dd646cebe4764b57d8f9947&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a06efb8c1dd646cebe4764b57d8f9947&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent ๐Ÿค– </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** docker-compose.yml
   **Line:** 111:111
   **Comment:**
   	*Api Mismatch: With no `DATABASE_PORT`, Postgres is published on host port 5433, but the development guide directs users to localhost:5432, so their connections miss this database.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45145&comment_hash=228b11155c7a4d2a7e0daf3a532b4e96a12d8df7d73c508fc69be56d8bebf6f3&reaction=like'>๐Ÿ‘</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45145&comment_hash=228b11155c7a4d2a7e0daf3a532b4e96a12d8df7d73c508fc69be56d8bebf6f3&reaction=dislike'>๐Ÿ‘Ž</a>



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4234176132


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,23 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+
 const d3NumberFormat: ControlFormItemSpec<'Select'> = {
   allowNewOptions: true,
   controlType: 'Select',
   label: t('D3 format'),
   description: D3_FORMAT_DOCS,
-  options: D3_FORMAT_OPTIONS.map(option => ({
+  options: d3NumberFormatOptions.map(option => ({
     value: option[0],
     label: option[1],
   })),
   defaultValue: D3_FORMAT_OPTIONS[0][0],
   creatable: true,
+  tokenSeparators: ['\n', '\t', ';'],

Review Comment:
   I kept the existing semicolon separator to avoid changing existing tokenization behavior beyond the scope of this fix. This change specifically ensures that commas remain part of D3 number format strings. I have also documented why commas are excluded from `tokenSeparators`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4230916356


##########
docker-compose.yml:
##########
@@ -107,7 +108,7 @@ services:
     image: postgres:17
     restart: unless-stopped
     ports:
-      - "127.0.0.1:${DATABASE_PORT:-5432}:5432"
+      - "127.0.0.1:${DATABASE_PORT:-5433}:5432"

Review Comment:
   Thanks for flagging this. The `docker-compose.yml` change was included accidentally and has since been removed from this PR. The PR is now focused only on the D3 number-format fix, so no Docker changes are needed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4233520451


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,23 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+
 const d3NumberFormat: ControlFormItemSpec<'Select'> = {
   allowNewOptions: true,
   controlType: 'Select',
   label: t('D3 format'),
   description: D3_FORMAT_DOCS,
-  options: D3_FORMAT_OPTIONS.map(option => ({
+  options: d3NumberFormatOptions.map(option => ({
     value: option[0],
     label: option[1],
   })),
   defaultValue: D3_FORMAT_OPTIONS[0][0],
   creatable: true,
+  tokenSeparators: ['\n', '\t', ';'],

Review Comment:
   Including `';'` as a token separator will break valid D3 format strings that use semicolons to separate positive/negative formats (e.g. `.2f;(.2f)`), because the input will be split into multiple tokens. Consider removing `';'` from `tokenSeparators` (and update the corresponding test), and rely on separators that cannot appear in a D3 specifier (e.g. newline/tab). Also, if users paste Windows newlines (`\\r\\n`), splitting only on `\\n` can leave a trailing `\\r` character in the token; consider handling `\\r` as well (either add `\\r` to separators or ensure tokens are trimmed/normalized).



##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,23 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+
 const d3NumberFormat: ControlFormItemSpec<'Select'> = {
   allowNewOptions: true,
   controlType: 'Select',
   label: t('D3 format'),
   description: D3_FORMAT_DOCS,
-  options: D3_FORMAT_OPTIONS.map(option => ({
+  options: d3NumberFormatOptions.map(option => ({
     value: option[0],
     label: option[1],
   })),
   defaultValue: D3_FORMAT_OPTIONS[0][0],

Review Comment:
   The control options are now derived from `d3NumberFormatOptions`, but `defaultValue` is still tied to `D3_FORMAT_OPTIONS`. To keep the default consistent with the rendered option list (and avoid drift if the source changes), consider deriving the default from `d3NumberFormatOptions[0][0]` instead.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4234889629


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,24 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+

Review Comment:
   Thanks for the suggestion! I've added `,.0f` to the global `D3_FORMAT_OPTIONS` and updated Column Config to use the shared options.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 merged PR #45145:
URL: https://github.com/apache/superset/pull/45145


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45145:
URL: https://github.com/apache/superset/pull/45145#issuecomment-6082016524

   ## CodeAnt PR Risk: Low Risk
   
   - The PR appears safe to merge; it adds a comma-formatted D3 preset and preserves commas during free-text entry.
   - Tests check the exact preset and the token separators used to prevent commas from splitting formats.
   
   **Assessed commit:** `d55c1fc5ddac`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4234180771


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,23 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+
 const d3NumberFormat: ControlFormItemSpec<'Select'> = {
   allowNewOptions: true,
   controlType: 'Select',
   label: t('D3 format'),
   description: D3_FORMAT_DOCS,
-  options: D3_FORMAT_OPTIONS.map(option => ({
+  options: d3NumberFormatOptions.map(option => ({
     value: option[0],
     label: option[1],
   })),
   defaultValue: D3_FORMAT_OPTIONS[0][0],

Review Comment:
   Addressed. I updated `defaultValue` to use `d3NumberFormatOptions[0][0]` so that the default value is derived from the same options list used by the D3 number format control.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45145:
URL: https://github.com/apache/superset/pull/45145#issuecomment-6081932250

   ### <span aria-hidden="true">โœ…</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">๐Ÿ”จ</span> Latest commit | ffacc147972787d9beda8364d8f970752d3961b1 |
   |<span aria-hidden="true">๐Ÿ”</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ac8ec0a822715000875f922 |
   |<span aria-hidden="true">๐Ÿ˜Ž</span> Deploy Preview | [https://deploy-preview-45145--superset-docs-preview.netlify.app](https://deploy-preview-45145--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">๐Ÿ“ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MTQ1LS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.4bjvj8TbShxKzjL64aNouFEI7hU1f9Ovi2fvE_0dOSE)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">๐Ÿค–</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6yQzTukHHGH8UNCpmUER3z91bvBdySZQrLAAdmILRbs1hv3s3sniR8YWFVAgrs4kIz9acz0d6VtfihFZ7y1ELoQbJOddKhlkI-H4) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45145--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45145:
URL: https://github.com/apache/superset/pull/45145#issuecomment-6082414698

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45145?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 82.80%. Comparing base ([`3bc9e9e`](https://app.codecov.io/gh/apache/superset/commit/3bc9e9e479f15830d56993815c6a8d0ecd73bc37?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d55c1fc`](https://app.codecov.io/gh/apache/superset/commit/d55c1fc5ddacc306b5d10f434ccaa94ae4a49096?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #45145   +/-   ##
   =======================================
     Coverage   82.79%   82.80%           
   =======================================
     Files        3016     3016           
     Lines      192511   192512    +1     
     Branches    44840    44840           
   =======================================
   + Hits       159399   159400    +1     
     Misses      30079    30079           
     Partials     3033     3033           
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45145/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ” | |
   |---|---|---|
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/45145/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.53% <100.00%> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45145?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4234540521


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,24 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+

Review Comment:
   Can this be added globally, rather than separately for this specific case ( in superset-frontend/packages/superset-ui-chart-controls/src/utils/D3Formatting.ts)?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(column-config): allow commas in D3 number formats [superset]

Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #45145:
URL: https://github.com/apache/superset/pull/45145#discussion_r4233163471


##########
superset-frontend/src/explore/components/controls/ColumnConfigControl/constants.tsx:
##########
@@ -46,17 +46,23 @@ export type SharedColumnConfigProp =
   | 'displayTypeIcon'
   | 'currencyFormat';
 
+const d3NumberFormatOptions: [string, string][] = [
+  ...D3_FORMAT_OPTIONS,
+  [',.0f', ',.0f (12,345)'],
+];
+
 const d3NumberFormat: ControlFormItemSpec<'Select'> = {
   allowNewOptions: true,
   controlType: 'Select',
   label: t('D3 format'),
   description: D3_FORMAT_DOCS,
-  options: D3_FORMAT_OPTIONS.map(option => ({
+  options: d3NumberFormatOptions.map(option => ({
     value: option[0],
     label: option[1],
   })),
   defaultValue: D3_FORMAT_OPTIONS[0][0],
   creatable: true,
+  tokenSeparators: ['\n', '\t', ';'],

Review Comment:
   In https://github.com/apache/superset/pull/37454, `tokenSeparators` was added, excluding only the comma; a comment indicating the location of the original constant was also added.
   
   `// default value tokenSeparators in superset-frontend/packages/superset-ui-core/src/components/Select/constants.ts
     tokenSeparators: ['\r\n', '\n', '\t', ';'],`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org