Skip to content

Make the HERE Map display in the currently selected language#9552

Merged
ahukkanen merged 2 commits intodecidim:developfrom
belighted:feature/make-here-map-use-current-language
Jul 15, 2022
Merged

Make the HERE Map display in the currently selected language#9552
ahukkanen merged 2 commits intodecidim:developfrom
belighted:feature/make-here-map-use-current-language

Conversation

@sergei-krylov
Copy link
Copy Markdown
Contributor

@sergei-krylov sergei-krylov commented Jul 8, 2022

🎩 What? Why?

Make the HERE Map display in the currently selected language.
See the supported languages in docs: https://developer.here.com/documentation/map-tile/dev_guide/topics/resource-base-maptile.html#resource-base-maptile__includes-query-parameters

Testing

Find any map on the app, on meetings page as an example, choose another language for website to see the map also changes its language (if the language is one of the supported).

Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Really nice improvement @sergei-krylov, thanks a lot!

I tested it locally and it works just as advertised, although I did not understand why looking at the HERE tile layer code for Leaflet. But I guess I don't need to care how it works if it just works. 😆

One change I'd like to propose is to move the added logic to the Ruby backend as suggested here in the review. My intention would be to add some specs for this (e.g. test the officially supported locales en, ca, es) and it might be easier to implement these if the logic is at the backend.

We'd also like to keep the JS code for the maps initialization as simple as possible, which is why we created the maps API in the first place which allows providing options to the view/JS.

@sergei-krylov
Copy link
Copy Markdown
Contributor Author

sergei-krylov commented Jul 12, 2022

@ahukkanen , thanks a lot for review of this PR!

I moved everything to the backend as suggested.
Also, current_locale is not accessible in that builder as it is sitting in /lib folder, so no view helpers available for it. I used the I18n.locale instead which normally should refer to either the current language or target language (kind of 18n.with_locale(...) calls) for usages of the builder.

@sergei-krylov sergei-krylov requested a review from ahukkanen July 12, 2022 11:58
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Looking pretty good! I still left one more request to fix the Rubocop issues, would be a bit more readable.

@ahukkanen
Copy link
Copy Markdown
Contributor

Also, current_locale is not accessible in that builder as it is sitting in /lib folder, so no view helpers available for it. I used the I18n.locale instead which normally should refer to either the current language or target language (kind of 18n.with_locale(...) calls) for usages of the builder.

It would be actually available through the @template object available in the Builder sub-class (not the provider class itself) as stated in the original review but essentially what current_locale is doing is exactly what you are already doing, so I don't mind the current approach either.

If we can just fix those Rubocop issues as suggested in the latest review, it's good to go (I'll still need to test it once more after finished).

@ahukkanen ahukkanen added module: core type: fix PRs that implement a fix for a bug labels Jul 13, 2022
@sergei-krylov sergei-krylov requested a review from ahukkanen July 14, 2022 14:23
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen left a comment

Choose a reason for hiding this comment

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

Great improvement, thanks a lot!

@ahukkanen ahukkanen merged commit 877341a into decidim:develop Jul 15, 2022
entantoencuanto added a commit that referenced this pull request Jul 15, 2022
…ging

* feature/redesign-main-footer:
  Reorder elements in main links of footer and define links and texts
  Define a cell for static_pages and topics configured to appear in footer
  Fix translation call
  Set fixed links in redesigned_main_legal partial
  Add FooterMenuPresenter to display menu items in footer
  Fix budgets seeds on non development apps (#9585)
  Return 404 when there isn't a valid component in program (#9576)
  Add missing queue close_meeting_reminder to sidekiq configuration (#9568)
  Make the HERE Map display in the currently selected language (#9552)
  Add help text for proposals' 'publish answers immediately' setting  (#9549)
  Fix admin language selector with more than 4 locales (#9519)
  Fix publish event on official proposals (#9421)
  Prevent missing ActionLog entries to break the application (#9502)
  Add boilerplate structure to CHANGELOG (#9501)
  Add step-by-step instructions of the Crowdin releases process (#9555)
  Fix translated attributes field type change (#9547)
  Add `modifyList` option to the autocomplete element (#9548)
  Admin log filters (#9460)
  Improve the default gitignore files created by the generators (#9507)
eliegaboriau pushed a commit to eliegaboriau/decidim that referenced this pull request Oct 25, 2022
…#9552)

* Make the HERE map display in the currently selected language

* Refactor HERE map language codes method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: core type: fix PRs that implement a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants