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

feat(formatters): Add custom d3-time-format locale #24263

Merged
merged 27 commits into from
Jun 10, 2024

Conversation

matheusbsilva
Copy link
Contributor

@matheusbsilva matheusbsilva commented Jun 1, 2023

SUMMARY

Adds a configuration option to change the default language of time related data on charts, including the option to translate months. This is related to issues: #3972 , #13442 and #17447.

During the implementation I had to change how the smart formatters were used across the application, because I had to ensure that they were instantiated with the right locale configuration. So instead of importing them directly, I got them through the TimeFormatterRegistry. I'm not sure if is the best option, let me know.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Without time format configuration:
Captura de tela de 2023-06-01 09-49-00

With brazilian portuguese time format configuration:
Captura de tela de 2023-06-01 09-47-58

TESTING INSTRUCTIONS

Add the D3_TIME_FORMAT with your locale configuration to superset_config.py following the definition expected by D3 (docs), for example, this is the definition for brazilian portuguese:

D3_TIME_FORMAT = {
  "dateTime": "%A, %e de %B de %Y. %X",
  "date": "%d/%m/%Y",
  "time": "%H:%M:%S",
  "periods": ["AM", "PM"],
  "days": ["Domingo", "Segunda", "Terça", "Quarta", "Quinta", "Sexta", "Sábado"],
  "shortDays": ["Dom", "Seg", "Ter", "Qua", "Qui", "Sex", "Sáb"],
  "months": ["Janeiro", "Fevereiro", "Março", "Abril", "Maio", "Junho", "Julho", "Agosto", "Setembro", "Outubro", "Novembro", "Dezembro"],
  "shortMonths": ["Jan", "Fev", "Mar", "Abr", "Mai", "Jun", "Jul", "Ago", "Set", "Out", "Nov", "Dez"]
}

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@matheusbsilva matheusbsilva changed the title [WIP] feat(formatters): Add custom d3-time-format locale feat(formatters): Add custom d3-time-format locale Jun 1, 2023
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #24263 (3ad0d63) into master (0836000) will decrease coverage by 0.02%.
The diff coverage is 63.43%.

❗ Current head 3ad0d63 differs from pull request most recent head 38b4890. Consider uploading reports for the commit 38b4890 to get more accurate results

@@            Coverage Diff             @@
##           master   #24263      +/-   ##
==========================================
- Coverage   69.10%   69.08%   -0.02%     
==========================================
  Files        1906     1904       -2     
  Lines       74175    74155      -20     
  Branches     8163     8173      +10     
==========================================
- Hits        51259    51232      -27     
- Misses      20788    20806      +18     
+ Partials     2128     2117      -11     
Flag Coverage Δ
hive 54.15% <44.70%> (+0.21%) ⬆️
mysql 79.48% <84.70%> (+0.09%) ⬆️
postgres 79.57% <84.70%> (+0.09%) ⬆️
presto 54.04% <44.70%> (+0.21%) ⬆️
python 83.54% <85.88%> (+0.06%) ⬆️
sqlite 78.13% <72.35%> (+0.09%) ⬆️
unit 54.83% <48.23%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
...perset-ui-chart-controls/src/utils/D3Formatting.ts 100.00% <ø> (ø)
...t-ui-core/src/time-format/TimeFormatterRegistry.ts 100.00% <ø> (ø)
.../src/time-format/factories/createMultiFormatter.ts 100.00% <ø> (ø)
...plugins/legacy-plugin-chart-heatmap/src/Heatmap.js 0.00% <ø> (ø)
.../legacy-plugin-chart-heatmap/src/transformProps.js 0.00% <0.00%> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <ø> (ø)
...egacy-plugin-chart-world-map/src/transformProps.js 0.00% <0.00%> (ø)
...nd/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js 0.00% <ø> (ø)
...tend/plugins/legacy-preset-chart-nvd3/src/utils.js 15.38% <ø> (-0.54%) ⬇️
...harts/src/BigNumber/BigNumberTotal/controlPanel.ts 30.00% <ø> (ø)
... and 93 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@eschutho
Copy link
Member

eschutho commented Jun 9, 2023

Also ccing @villebro since he is working on some other localization features.

@matheusbsilva
Copy link
Contributor Author

@rusackas I added more tests to fix the coverage issue reported in the last CI run.

@matheusbsilva
Copy link
Contributor Author

@rusackas Is there anything else that I can do to help with the review?

@rusackas
Copy link
Member

/testenv up

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM, but would love a second opinion from @kgabryje or @justinpark or @michael-s-molina or @villebro

I know this PR will make a lot of people happy when it's merged, too!

Copy link
Contributor

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

@matheusbsilva
Copy link
Contributor Author

up

@rusackas
Copy link
Member

rusackas commented May 7, 2024

Running CI again - fingers crossed! Last chance, @kgabryje / @justinpark / @michael-s-molina / @villebro

@matheusbsilva
Copy link
Contributor Author

@rusackas I fixed some linting errors caused by merge conflict.

@matheusbsilva
Copy link
Contributor Author

up

@rusackas
Copy link
Member

rusackas commented Jun 4, 2024

CI running again 🤞 . Feel free to DM me on slack as well if you need me to push buttons here again ;)

@BaraDiaw
Copy link

BaraDiaw commented Jun 7, 2024

Hello, I've been using superset for 3 months. I want to change the language to french. I added the D3_TIME_FORMAT with my local configuration for superset_config.py but it still doesn't work. Could you help me again.
Capture d'écran 2024-06-07 122437

@cchristofr
Copy link

cchristofr commented Jun 7, 2024 via email

@BaraDiaw
Copy link

BaraDiaw commented Jun 7, 2024

thanks @cchristofr for your reply, I'll keep waiting then

@cchristofr
Copy link

cchristofr commented Jun 7, 2024 via email

@BaraDiaw
Copy link

BaraDiaw commented Jun 7, 2024

Non, je travaille au Sénégal.

@cchristofr
Copy link

cchristofr commented Jun 7, 2024 via email

@rusackas
Copy link
Member

rusackas commented Jun 7, 2024

Closing and re-opening due to a weird glitch with E2E tests.

@rusackas rusackas closed this Jun 7, 2024
@rusackas rusackas reopened this Jun 7, 2024
Copy link
Contributor

github-actions bot commented Jun 7, 2024

Ephemeral environment shutdown and build artifacts deleted.

Copy link
Member

@sfirke sfirke left a comment

Choose a reason for hiding this comment

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

I can't speak to the code details but everything looked good to me and it's a great feature!

@rusackas rusackas merged commit 024cfd8 into apache:master Jun 10, 2024
61 of 64 checks passed
@Zhikharevi
Copy link

Sorry, could you help? I paste your code, but for some reason nothing changes

Снимок экрана 2024-08-12 в 14 58 17 Снимок экрана 2024-08-12 в 14 59 30

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 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 dependencies:npm packages plugins size/XXL 🚢 4.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.