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

Update text domain to eight-day-week-print-workflow #41

Merged
merged 11 commits into from
Nov 13, 2019

Conversation

jeffpaul
Copy link
Member

@jeffpaul jeffpaul commented Jul 31, 2019

Description of the Change

The Text Domain is currently set to eight-day-week, but needs to be eight-day-week-print-workflow to match the plugin slug on WordPress.org in order for translations to be open on translate.wordpress.org. This PR updates the Text Domain to eight-day-week-print-workflow.

Alternate Designs

none

Benefits

Translations will now be open for contributions on translate.wordpress.org.

Possible Drawbacks

none identified

Verification Process

Manually verified via GitHub web UI.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Relates to #40.

@jeffpaul jeffpaul added this to the 1.2.0 milestone Jul 31, 2019
@jeffpaul jeffpaul self-assigned this Jul 31, 2019
@jeffpaul
Copy link
Member Author

I suspect eight-day-week-de_DE.mo might also need an edit for the .org slug but I'm unsure whether that filename also needs to be updated (same for the PO and POT files). I'm going to read some more on .org translations before this is likely ready for review.

@jeffpaul jeffpaul changed the title Update text domain to eight-day-week-print-workflow [WIP] Update text domain to eight-day-week-print-workflow Jul 31, 2019
@jeffpaul
Copy link
Member Author

So I think that captures the various changes called out as needed on https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/. Certainly needs reviewing to see if anything was missed or incorrectly updated. Fun.

@jeffpaul jeffpaul changed the title [WIP] Update text domain to eight-day-week-print-workflow Update text domain to eight-day-week-print-workflow Aug 1, 2019
load_textdomain( 'eight-day-week', WP_LANG_DIR . '/print-production/print-production-' . $locale . '.mo' );
load_plugin_textdomain( 'eight-day-week', false, plugin_basename( EDW_PATH ) . '/languages/' );
$locale = apply_filters( 'plugin_locale', get_locale(), 'eight-day-week-print-workflow' );
load_textdomain( 'eight-day-week-print-workflow', WP_LANG_DIR . '/eight-day-week-print-workflow/eight-day-week-print-workflow-' . $locale . '.mo' );
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm least confident in this change here, so confirmation that the concatenation of this all makes sense would be good.

Choose a reason for hiding this comment

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

I think this is right, it works correctly locally. I can check some other plugins as well to confirm the approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this anymore if trying to use WordPress.org language packs? It means bumping the WP required version (4.6+ I believe) and then we can remove the text domain header. I'm going to ask somebody who knows more about how the .org side about why this doesn't work with the custom text domain header set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, not super urgent to get these changes handled, just more a "nice to check it off the list" sort of thing.

@adamsilverstein
Copy link

adamsilverstein commented Aug 8, 2019

@jeffpaul I added a few missing textdomain replacements and renamed the translation files.

Testing

I tested this locally by setting my profile language to German and checking that all our custom strings are properly translated. They all looked good, other than a few that are hard coded into our JavaScript (for example https://github.com/10up/eight-day-week/blob/develop/assets/js/src/scripts.js#L356) - these can wither use JS translation functions or be localized from PHP and can be handled in a follow up PR.

adamsilverstein
adamsilverstein previously approved these changes Aug 8, 2019
Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Swell

@jeffpaul jeffpaul added the type:bug Something isn't working. label Aug 8, 2019
@jeffpaul
Copy link
Member Author

jeffpaul commented Aug 27, 2019

Pascal Casier (@casiepa on WordPress.org) shared this tool to help highlight what's needed to prep plugins for i18n: http://wp-info.org/tools/checkplugini18n.php?slug=eight-day-week-print-workflow. That plus confirmation from Tor-Björn Fjellner and Sergey Biryukov in the WordPress Slack #pluginreview channel seems to show that we need to:

  • change our Text Domain in eight-day-week.php so it is equal to our slug
  • modify the text domain in all our source files
  • either bump "requires at least" to 4.6 and remove the load_plugin_textdomain() call or correct the load_plugin_textdomain() call

@jeffpaul
Copy link
Member Author

I believe that the changes in this PR from me and @adamsilverstein cover the first two checklist items above, so I'm marking those off. The final checklist item relates to our existing conversation, I'd say that we bump to 4.6 and remove that call but want to hear what others prefer / think?

@jeffpaul jeffpaul modified the milestones: 1.2.0, 1.1.1 Sep 6, 2019
@jeffpaul
Copy link
Member Author

Updated "Requires at least" to version 4.6, this should be the last task to have this PR complete to allow proper translations. Based on @adamsilverstein's prior approval and final tweaks, I'll merge and include this alongside the "Tested up to" bump to version 5.3.

@jeffpaul jeffpaul merged commit e6e071e into develop Nov 13, 2019
@jeffpaul jeffpaul deleted the fix/text-domain branch September 30, 2020 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants