-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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(urlParam): new locale urlparam embedding to setup language for traductions #31646
base: master
Are you sure you want to change the base?
feat(urlParam): new locale urlparam embedding to setup language for traductions #31646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've completed my review and didn't find any issues... but I did find this horse.
.''
._.-.___.' (`\
//( ( `'
'/ )\ ).__. )
' <' `\ ._/'\
` \ \
Files scanned
File Path | Reviewed |
---|---|
superset/app.py | ✅ |
superset-frontend/src/components/Loading/index.tsx | ✅ |
superset-frontend/src/constants.ts | ✅ |
superset/views/base.py | ✅ |
superset/views/core.py | ✅ |
superset/config.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-review
on this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-review
command in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-description
command in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolve
command in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31646 +/- ##
===========================================
+ Coverage 60.48% 83.75% +23.26%
===========================================
Files 1931 538 -1393
Lines 76236 39161 -37075
Branches 8568 0 -8568
===========================================
- Hits 46114 32800 -13314
+ Misses 28017 6361 -21656
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Looks like this might just need |
@rusackas alright, I’ll try this approach. I spent some time last week working on currency management using the "locale" key in the urlParams. I'll push the code to this branch this afternoon; it’ll be cleaner than creating a separate independent PR. |
@rusackas for the company I work with, and I believe others in the community might also need this, I would like to make it possible to translate chart titles on a dashboard. This is something that isn’t currently dynamic with the language selection. Have there been any ideas or discussions on this topic? |
@ERGO1995 could you please add a link to the associated issue in the additional information? |
Running CI, but it doesn't look happy so far. Regarding translations for "dynamic content" (chart titles, dashboard titles, column names, dataset names, etc., etc.) there hasn't yet been a SIP around this. It's been talked about, but nothing formalized. Some people want to do it with automated API based workflows, some want to just have a translations table that uses UUIDs for all assets as a reference, some want to commingle it with the current translation files/build process. Unclear if it should be tied to any new UI, or if the process is "invisible". If you have ideas, contributions/discussion would be welcomed. |
@rusackas I tested this modification in charts.jsx:
Then, I created a file superset/superset-frontend/src/dashboard/customTrads.ts like this:
Next, I executed the following commands in the backend container:
Afterward, I recompiled the translations in both the backend and frontend, as the chart titles in the dashboards rely on the messages.json file. Now, when selecting the language, the translations I configured in messages.po are correctly applied once extracted. I'm not sure if this is the best approach, but with minimal changes to the existing code and using a custom file, it seems to achieve the desired outcome. |
@rusackas Regarding the CI, it seems like it doesn't like the declaration of local-currency. Maybe it's not in the correct place where I declared it? It was working locally, though. |
SUMMARY
I picked up an existing PR that was no longer progressing (PR 30318). The idea is to make the language dynamic and configurable in the context of embedding a dashboard. To achieve this, I added a "locale" key in the urlParams, which allows passing the desired language code. Thus, in an embedding context, you just need to manage this code dynamically so that the user can view the dashboards in their preferred language.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
locale: 'en' :
locale: 'fr'
TESTING INSTRUCTIONS
You can modify the "locale" key here:
ADDITIONAL INFORMATION