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(range-slider): removed localization of metric key #24716

Conversation

Always-prog
Copy link
Contributor

@Always-prog Always-prog commented Jul 17, 2023

SUMMARY

Due to the this localization, the Range Slider is not functioning properly because we are unable to retrieve the min and max variables from the data because it was translated.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Example with a russian localization

Before

Screenshot_59 Message in english: a non-numeric column is selected

After

Screenshot_61

TESTING INSTRUCTIONS

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

@john-bodley
Copy link
Member

Thanks @Always-prog for the PR. Would you mind expanding the PR description to include a before and after screenshot? This would help the reviewers better grok the change.

@Always-prog
Copy link
Contributor Author

It's just a simple problem, so there isn't much to explain about. But I have added the screenshots.

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.

It's just a simple problem, so there isn't much to explain about. But I have added the screenshots.

Thank you for adding the screenshots @Always-prog. Screenshots are helpful for reviewers and they provide a lot of context to understand the problem. Take your original description for example:

Due to the this localization, the Range Slider is not functioning properly because we are unable to retrieve the min and max variables from the data because it was translated.

The term "Range Slider" can mean many different components in the application. The screenshot help us quickly understand that you're actually talking about the Numerical range filter type. A reviewer that is not very familiar with that part of the application may not get this information quickly by only looking at the code 😉

LGTM! Thank you for the fix!

@michael-s-molina michael-s-molina merged commit 2d58ddd into apache:master Jul 19, 2023
29 checks passed
@john-bodley
Copy link
Member

Thanks @Always-prog for the updated PR description. Echoing @michael-s-molina's comment, looking at the change sans context meant it was hard to grok whether the min and max words represented labels (which need to be translated) or internal attributes.

@Always-prog
Copy link
Contributor Author

@john-bodley okay, got it!

@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 20, 2023
michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023
@mistercrunch mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 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 size/XS v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants