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.

Sign up for GitHub

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

Jump to bottom

Gallery Styles: Load different style based on language #959

Closed
wants to merge 1 commit into from
Closed

Gallery Styles: Load different style based on language #959

wants to merge 1 commit into from

Conversation

louwers
Copy link
Collaborator

@louwers louwers commented Nov 24, 2024 •
edited
Loading

  • Update Protomaps Light to v4.
  • Allow loading different (default) style based on selected language. It will replace $lang in the style URL with the selected language.
  • E2E test with Cypress that confirms the correct style is loaded depended on selected language.
Screenshot 2024-11-24 at 16 37 25

Launch Checklist

  • Briefly describe the changes in this PR.
  • Link to related issues. N/A
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

@louwers louwers requested a review from HarelM November 24, 2024 16:12
@HarelM
Copy link
Collaborator

HarelM commented Nov 24, 2024

Thanks for taking the time to open this PR! but I don't think this is a good UX.
I'll explain why:
Maputnik is about allowing editors to see how their map would look.
If I need to change the language all I need to do is load a different style.
If I want to edit a map related to a different language I usually won't need the UI of Maputnik to change as well, on the contrary, this would probably create worse experience as a designer as I'll have hard time finding the right buttons and panels...
I'm not saying we shouldn't think about better experience to support multiple map languages, but this is not exactly it.

Consider the following somewhat equivalent: you are writing a Google docs document, and every time you want to write in a different language all the UI changes according to the current language.

@louwers
Copy link
Collaborator Author

louwers commented Nov 24, 2024 •
edited
Loading

@HarelM This does not affect any styles the user loads when inserting an URL. It only affects which style is loaded when selecting the "ProtoMaps Light" default style1.

Using your analogy, if I use Google Docs and I go to "Document Templates" I expect those templates to match the interface language. And in fact it does:

image image

Footnotes

  1. If any of the other default styles support other languages as well, we can update them to load the language that the user has selected. ↩

@louwers louwers changed the title Allow loading different style based on language Load different default style based on language Nov 24, 2024
@louwers louwers changed the title Load different default style based on language Gallery Styles: Load different style based on language Nov 25, 2024
@louwers
Copy link
Collaborator Author

louwers commented Nov 25, 2024

Updated title to be more clear.

HarelM
HarelM reviewed Nov 25, 2024
View reviewed changes
src/config/styles.json
@@ -80,7 +80,7 @@
{
"id": "protomaps-light",
"title": "Protomaps Light",
"url": "https://api.protomaps.com/styles/v2/light.json?key=d828297496b11844",
"url": "https://api.protomaps.com/styles/v4/light/$lang.json?key=d828297496b11844",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the common usage is {lang} and not $lang in maplibre - glyphs, tiles etc use "{}" for parameters.

HarelM
HarelM reviewed Nov 25, 2024
View reviewed changes
src/components/ModalOpen.tsx
@@ -56,6 +56,10 @@ type ModalOpenState = {
activeRequestUrl?: string | null
};

function transformStyleUrl(styleUrl: string, {language}: { language: string }) {
return styleUrl.replace("$lang", language);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happen if the url doesn't support the relevant language?

HarelM
HarelM reviewed Nov 25, 2024
View reviewed changes
cypress/e2e/i18n.cy.ts

describe("load different style depending on selected language", () => {
const testLang = (lang: string) => {
cy.intercept('GET', `**/${lang}.json*`).as(`${lang}JsonRequest`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

cy specific code should be moved to the driver.

@HarelM
Copy link
Collaborator

HarelM commented Nov 25, 2024

Thanks for the info.
It's still not clear to me how one can use different languages from the protomaps styles, but it might be a different feature and out of scope for this PR.
I better understand the concept around this feature now, thanks!

@louwers
Copy link
Collaborator Author

louwers commented Nov 25, 2024 •
edited
Loading

I will address this differently in another PR so that variations (other languages) of the gallery style can be chosen.

I agree that we shouldn't limit ourselves to the languages that happen to be supported by our interface.

@louwers louwers closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HarelM HarelM HarelM left review comments

Assignees
No one assigned
Labels
enhancement i18n
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@louwers @HarelM