locale query string support on public links and static embeds #47186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
npretto
added
no-backport
npretto
changed the title
kulyk
reviewed
Aug 27, 2024
There was a problem hiding this comment.
Why are we skipping these three tests in that file?
| @@ -122,7 +122,7 @@ export function useDashboardUrlQuery( | |||
| }, [router, location, selectedTab, dispatch]); | |||
| } | |||
|
|
|||
| const QUERY_PARAMS_ALLOW_LIST = ["objectId"]; | |||
| const QUERY_PARAMS_ALLOW_LIST = ["objectId", "locale"]; | |||
There was a problem hiding this comment.
I wonder if we should also update the parameters allow list for the SyncedParametersList component (used for native question parameters)?
There was a problem hiding this comment.
💯 yes!
Thank you for pointing that out, let me change one of the e2e tests to cover that scenario
There was a problem hiding this comment.
Awesome! It'd be great to unify this logic at some point in the future, but it's kind of hard to do while native question parameters are using history instead of react-router-redux. #44639 should have more context around that
Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com>
WiNloSt
reviewed
Aug 28, 2024
| // > `url` - one that begins with `http`. By default `cy.request()` will use | ||
| // > either the current window's origin or the `baseUrl` in | ||
| // > `e2e/support/cypress.config.js`. Neither of those values were present. | ||
| Cypress.config("baseUrl", originalBaseUrl); |
There was a problem hiding this comment.
For anyone who is still confused about this line. Because of the way Cypress.config works, it only is reset when the whole suite is completed, so this is a way to restore the value back in between tests.
By the way, it's used here.
Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
WiNloSt
approved these changes
Aug 29, 2024
npretto
added a commit
that referenced
this pull request
Aug 29, 2024
* keep locale in url query params * e2e tests * ugly fix for the missing baseUrl error * applies suggestion from Kelvin to make code more demure * Update e2e/test/scenarios/sharing/public-question.cy.spec.js Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> * remove it.skip added by mistake * sort imports * handle native question/ SyncedParametersList too * shorter and more accurate comment 😅 * Update e2e/support/helpers/e2e-embedding-helpers.js Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> --------- Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
qnkhuat
pushed a commit
that referenced
this pull request
Aug 30, 2024
* keep locale in url query params * e2e tests * ugly fix for the missing baseUrl error * applies suggestion from Kelvin to make code more demure * Update e2e/test/scenarios/sharing/public-question.cy.spec.js Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> * remove it.skip added by mistake * sort imports * handle native question/ SyncedParametersList too * shorter and more accurate comment 😅 * Update e2e/support/helpers/e2e-embedding-helpers.js Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> --------- Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
npretto
added a commit
that referenced
this pull request
Aug 30, 2024
…embeds" (#47393) * locale query string support on public links and static embeds (#47186) * keep locale in url query params * e2e tests * ugly fix for the missing baseUrl error * applies suggestion from Kelvin to make code more demure * Update e2e/test/scenarios/sharing/public-question.cy.spec.js Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> * remove it.skip added by mistake * sort imports * handle native question/ SyncedParametersList too * shorter and more accurate comment 😅 * Update e2e/support/helpers/e2e-embedding-helpers.js Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> --------- Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * fix tests, the button i was looking for is not on v50 --------- Co-authored-by: Nicolò Pretto <info@npretto.com> Co-authored-by: Anton Kulyk <kuliks.anton@gmail.com> Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr addresses Support language localization in static embedding.
The feature was apparently already working with one small caveat: we have some code that removes all querystring parameters that are not filters, this refreshing the page losing the locale as reported by Jeff on slack
The rest of the code changes are tests for the 4 different contexts (public/static, question/dashboard)