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

Config: Extract languages to shared config and remove obsolete muse_supported_themes #5413

Merged
merged 2 commits into from
May 25, 2016

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 17, 2016

We repeat across all config files so many values. We could easily mitigate that issue by using already present _shared.json config file introduced by @mjangda in #876. This PR tries to slim down config files by extracting muse_supported_themes and languages fields. We probably can open the follow up PR with more changes.

Testing

  1. Run npm test and make sure everything passes.
  2. Run http://calypso.localhost:3000/me/account and make sure you still can change language for your interface :)
  3. Go to your site's setting page and check if you can change language of your site.

@gziolo gziolo self-assigned this May 17, 2016
@gziolo
Copy link
Member Author

gziolo commented May 17, 2016

/cc @mtias @rralian @mjangda @artpi

@gziolo gziolo added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 17, 2016
{ "value": 13, "langSlug": "cy", "name": "cy - Cymraeg" },
{ "value": 14, "langSlug": "da", "name": "da - Dansk" },
{ "value": 15, "langSlug": "de", "name": "de - Deutsch", "popular": 4 },
{ "value": 427, "langSlug": "dv", "name": "dv - ދިވެހިބަސް" },
Copy link
Member Author

Choose a reason for hiding this comment

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

Can someone double check if that works properly for users? Everything else displays properly for me, but this one seems to be broken.

@mtias
Copy link
Member

mtias commented May 17, 2016

I haven't tested the language ones, but it's looking good to me.

@gziolo
Copy link
Member Author

gziolo commented May 18, 2016

It turned out muse_supported_themes value is no longer referenced in code. I removed it. @sirbrillig can you confirm that change?

@akirk can you double check language related change?

@sirbrillig
Copy link
Member

@gziolo wow, I did some digging and the use-case for muse_supported_themes was removed on August 4, 2015. (It had been added on Mar 4, 2015 to prevent showing any customize link in the sidebar (client/my-sites/sidebar/sidebar.jsx) when Muse would not be available, because the native customizer experience at mobile width was so poor.)

I'd say removing it is fine.

@gziolo gziolo changed the title Config: Extract muse_supported_themes and languages to shared config Config: Extract languages to shared config and remove obsolete muse_supported_themes May 25, 2016
@gziolo
Copy link
Member Author

gziolo commented May 25, 2016

@mtias can we land this one? It looks like there are no blockers.

@mtias
Copy link
Member

mtias commented May 25, 2016

Yeah, let's 🚢

@mtias mtias added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 25, 2016
@gziolo gziolo merged commit 531b4a9 into master May 25, 2016
@gziolo gziolo deleted the update/extract-config-shared-values branch May 25, 2016 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants