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

Set the map language to UI language in new branch #797

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WindLi001
Copy link
Contributor

This is a solution of #789. The language of map in Place will be set to the same one of the UI. If the UI language is NOT supported by Mapbox, then the English map will be provided.
This is a new pull request of Set the map language to UI language, and I commit it to a new branch other than master. The old one will be closed soon.
As the comment in the old pull request #790, I delete the setting of zoom and center, and delete the setting of projection, which make the map language cannot be changed.
I have a look to the commit setting the projection, it said "chore: add mapbox globe view", so I guess the committer just want to show the mapbox with whole world map initially. But I think maybe he or she don't know what the projection really means, in fact global projection just make a spherical map. So, I add zoom: 1 to show the mapbox with whole world map initially.

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Attention: Patch coverage is 10.00000% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 65.29%. Comparing base (1795ff1) to head (04b88fa).
Report is 43 commits behind head on master.

Files Patch % Lines
ui/src/localization.ts 8.77% 52 Missing ⚠️
ui/src/components/mapbox/MapboxMap.tsx 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
- Coverage   65.37%   65.29%   -0.09%     
==========================================
  Files         138      138              
  Lines       12891    12951      +60     
  Branches      533      533              
==========================================
+ Hits         8428     8456      +28     
- Misses       4303     4339      +36     
+ Partials      160      156       -4     
Flag Coverage Δ
api 36.20% <ø> (+1.29%) ⬆️
ui 69.69% <10.00%> (-0.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kkovaletp
Copy link
Contributor

I've merged this PR into the #910 - a temporary alternative to the master while the project is on hold, so users still could get the latest Photoview improvements and fixes by building an image locally from this PR code

@viktorstrate
Copy link
Member

I really like this addition, and I think it would be a good idea to merge it. But I think we recently added Ukrainian which should probably be added before we can merge this.

@kkovaletp
Copy link
Contributor

kkovaletp commented Mar 29, 2024

I think we recently added Ukrainian which should probably be added before we can merge this.

Oh, didn't expect that there might be an ordering issue (and GitHub doesn't show any conflicts with master now).
@jordy2254 , please review this PR and check the merged commit a90f303

Do you see an issue with merging this PR after that commit is done?
If yes, what would be the best way of solving this issue now? Reverting the branch to some earlier commit will also revert other merged PRs and we'll need to deal with that as well...

@jordy2254
Copy link
Member

@kkovaletp Merge conflicts unfortunatly don't work in that way. A merge conflict happens when 2 people edit the same area of code, in this circumstance neither have modified the same area.

@WindLi001 I have added a comment to make this more generic and add a tie for this to the language selection using a map which should make it so that people adding languages in the future are less likely to miss it. A test could be written for this to ensure the userPreferences list is the same length as the number of map keys.

return
}

switch (map_language) {
Copy link
Member

Choose a reason for hiding this comment

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

We already have all of this information within the Userpreferences.tsx called languagePreferences. We should extract this and the elements in languagePreferences into a map within this file which is LanguageTranslation -> languageCode Although in this case it does appear that the Chinese traditional & simplified do not use the same code as we do within UserPreferences (You might need an object rather than one field in the map).

Rewriting it in this way gives us somewhere more central to pick up on language codes and means things don't get missed in the future. A test can also be written to ensure the language selection list length is the same as the map to stop someone doing something strange in the future.

@kkovaletp
Copy link
Contributor

@jordy2254 as the PR owner is not responding, but @viktorstrate and I think that this improvement is adding value to the Photoview, Can you please fork it to your repo (by a command, similar to git fetch upstream pull/797/head:797-map-lang), go required changes, and merge it to master instead of this PR?

BTW, this PR is 1 of a few still not merged from the #910, so we're close to our 1st milestone)

@jordy2254 jordy2254 added the help wanted Extra attention is needed label Apr 14, 2024
@kkovaletp kkovaletp added this to In progress in Roadmap via automation Apr 29, 2024
@kkovaletp kkovaletp moved this from In progress to Before next release in Roadmap Apr 29, 2024
@kkovaletp kkovaletp linked an issue May 4, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed high priority
Projects
Roadmap
Before next release
Development

Successfully merging this pull request may close these issues.

Please supports Non-English map in Place
4 participants