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

Feature: Rename the "Path:" text to "Location:" #12243

Merged
merged 14 commits into from
May 1, 2023
Merged

Feature: Rename the "Path:" text to "Location:" #12243

merged 14 commits into from
May 1, 2023

Conversation

memory-hunter
Copy link
Contributor

@memory-hunter memory-hunter commented Apr 30, 2023

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Rename the "Path:" text to "Location" #11950

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Open app ...
    2. Open any folder/file properties

Screenshots (optional)
Screenshot_1

@Lukiluc29
Copy link
Contributor

Should we therefore modify path by location but in different languages

@memory-hunter
Copy link
Contributor Author

memory-hunter commented Apr 30, 2023

Should we therefore modify path by location but in different languages

Originally, the text given in other languages except English, had Path: in them from the get-go, so it was intuitive to swap them out with Location straight away. And in other languages, I guess that applies too, but since the issue only stated the English part, localization should then follow by the localizers I assume

@Lukiluc29
Copy link
Contributor

So for example in French I can put "Emplacement"

@memory-hunter
Copy link
Contributor Author

So for example in French I can put "Emplacement"

If that's the appropriate translation, then I would say yes.

@Lukiluc29
Copy link
Contributor

Lukiluc29 commented Apr 30, 2023

So I replace all the words path with location or that "path" and "path:" ?

@memory-hunter
Copy link
Contributor Author

So I replace all the words path with location or that "path" and "path:" ?

No, only the part where you see Location:, therefore, only Path: should be replaced. Make sure to not mix up values with names.

@Lukiluc29
Copy link
Contributor

And why not the others

@memory-hunter
Copy link
Contributor Author

And why not the others

I may be misunderstanding, could you elaborate on what the previous question meant? Check the changed files and you will see what I changed, then follow that. That might help.

@Lukiluc29
Copy link
Contributor

I meant why not for example "Item Path" and that "Path:"

@memory-hunter
Copy link
Contributor Author

I meant why not for example "Item Path" and that "Path:"

I only changed what was highlighted and approved in the issue. Otherwise, good point. Also, We are not sure whether that line string of Item Path relates to an actual path (meaning it is named path correctly) or if the same change might be required.

@Lukiluc29
Copy link
Contributor

So what do I do I translate all or that "Path: "

@memory-hunter
Copy link
Contributor Author

So what do I do I translate all or that "Path: "

Best action to take is to wait for the main dev to see this pr, then answer to us and proceed then appropriately.

@Lukiluc29
Copy link
Contributor

Ok, so I just translate "Path:" into French and then we'll see the rest

@yaira2
Copy link
Member

yaira2 commented Apr 30, 2023

@Lukiluc29 congrats on your first pull request! I think we can go about this one a little differently, instead of modifying the existing strings, we should add a new string called "Location" and update the xaml file to use this string. In general we only make changes to the en-us resw file and let Crowdin take care of the other languages.

@memory-hunter
Copy link
Contributor Author

@Lukiluc29 congrats on your first pull request! I think we can go about this one a little differently, instead of modifying the existing strings, we should add a new string called "Location" and update the xaml file to use this string. In general we only make changes to the en-us resw file and let Crowdin take care of the other languages.

I see. I will make the according changes per those instructions.

@memory-hunter
Copy link
Contributor Author

@Lukiluc29 congrats on your first pull request! I think we can go about this one a little differently, instead of modifying the existing strings, we should add a new string called "Location" and update the xaml file to use this string. In general we only make changes to the en-us resw file and let Crowdin take care of the other languages.

I redid the task as you suggested. One really strange thing is that I simply replaced the Location to Path in en-GB but the commit shows some other changes and I didn't write these... I would like to know how did those changes appear there, maybe because of syncing?

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Congratulations on your first pull request, LGTM

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label May 1, 2023
@memory-hunter
Copy link
Contributor Author

Congratulations on your first pull request, LGTM

Thank you so much!

@yaira2 yaira2 merged commit ee0d3cb into files-community:main May 1, 2023
@memory-hunter memory-hunter deleted the path-to-location branch May 1, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Rename the "Path:" text to "Location"
4 participants