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

[l10n] Add Norwegian Bokmål (nb-NO) locale #27520

Merged
merged 4 commits into from
Dec 22, 2021
Merged

[l10n] Add Norwegian Bokmål (nb-NO) locale #27520

merged 4 commits into from
Dec 22, 2021

Conversation

wogsland
Copy link
Contributor

@wogsland wogsland commented Jul 30, 2021 •
edited
Loading

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 30, 2021 •
edited
Loading

No bundle size changes

Generated by 🚫 dangerJS against 63fb926

@oliviertassinari oliviertassinari changed the title Add Norwegian localization [l10n] Add Norwegian (noNO) locale Aug 8, 2021
@oliviertassinari oliviertassinari added the new feature New feature or request label Aug 8, 2021
@oliviertassinari oliviertassinari changed the title [l10n] Add Norwegian (noNO) locale [l10n] Add Norwegian Bokmål (nb_NO) locale Aug 8, 2021
@oliviertassinari oliviertassinari added the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Aug 8, 2021
@oliviertassinari

This comment has been minimized.

Sign in to view
This was referenced Nov 16, 2021
Closed
Merged
@mbrookes mbrookes removed the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Nov 25, 2021
@mbrookes mbrookes reopened this Nov 25, 2021
@mbrookes
Copy link
Member

#29485 (comment)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 25, 2021
@oliviertassinari oliviertassinari added the l10n localization label Dec 15, 2021
@oliviertassinari oliviertassinari changed the title [l10n] Add Norwegian Bokmål (nb_NO) locale [l10n] Add Norwegian Bokmål (nb-NO) locale Dec 15, 2021
@michaldudak
Copy link
Member

@wogsland the structure of the localization objects has changed in v5. Could you please rebase on the current master and correct the structure?

Also, please update the docs\src\pages\guides\localization\localization.md file with information about the newly added locale.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 17, 2021
@wogsland
Copy link
Contributor Author

@wogsland the structure of the localization objects has changed in v5. Could you please rebase on the current master and correct the structure?

Also, please update the docs\src\pages\guides\localization\localization.md file with information about the newly added locale.

I don't usually rebase, but it seems to have messed things up with Github. Here it shows a crazy number of commits and file changes, but just comparing branches only shows my changes: https://github.com/mui-org/material-ui/compare/master...wogsland:patch-1?expand=1

@michaldudak
Copy link
Member

We've had a number of directory renames in the meantime, so rebasing is harder than usual. The easiest way to make it work would be to copy your changes somewhere out of the workspace, reset --hard your branch to the current master, manually apply the changes again, and force push

@wogsland
Copy link
Contributor Author

Ready for review again.

@michaldudak
Copy link
Member

Looking good, thanks! One more thing to correct is code formatting. Could you please run yarn prettier and commit the changes?

spiftire
spiftire reviewed Dec 22, 2021
View reviewed changes
Copy link

@spiftire spiftire left a comment

Choose a reason for hiding this comment

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

Translation looks great =) Looking forward for the merge

michaldudak
michaldudak reviewed Dec 22, 2021
View reviewed changes
packages/mui-material/src/locale/index.ts
@@ -814,7 +814,7 @@ export const enUS: Localization = {
// if (type === 'previous') {
return 'Go to previous page';
},
},
}},
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert changes to the above lines? I don't know why they've been modified, but let's not introduce any changes beyond what's necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the template I copied and it was missing a closing curly bracket. If others try to add locales copying this they will also get an error.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough :)

@michaldudak michaldudak merged commit f7dcebd into mui:master Dec 22, 2021
@michaldudak
Copy link
Member

@wogsland thanks for working on this!

@wogsland wogsland deleted the patch-1 branch December 22, 2021 14:34
@oliviertassinari
Copy link
Member

@wogsland Thanks for bearing with us! Matt brought a new interesting dimension to this problem :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrookes mbrookes mbrookes left review comments

@spiftire spiftire spiftire left review comments

@michaldudak michaldudak michaldudak approved these changes

Assignees
No one assigned
Labels
l10n localization new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wogsland @mui-pr-bot @oliviertassinari @mbrookes @michaldudak @spiftire