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

Adding states for Benin(BJ) country 🇧🇯 #27217

Merged
merged 5 commits into from
Sep 12, 2020

Conversation

manutheblacker
Copy link
Contributor

@manutheblacker manutheblacker commented Aug 4, 2020

I've added some states for the Benin country

All Submissions:

Changes proposed in this Pull Request:

These changes add all states for Benin country to the list of WooCommerce Countries. It give the ability to the customers, to pick and select the right states while he is buying on a WooCommerce store.

How to test the changes in this Pull Request:

  1. Pick a product on your store
  2. On the checkout page select Benin as country
  3. If you have selected Benin as country, you will need to pick the state name you want to pick and then proceed the order.
  4. Go to the WooCommerce order for a Benin customer and you'll see the state name displayed into the order

Changelog entry

Adding states for Benin country 🇧🇯

I've added some states from the country : Benin
formating based on phpcs standard
i18n/states.php Outdated
@@ -172,6 +172,85 @@
),
'BH' => array(),
'BI' => array(),
'BJ' => array( // Benin states.
'BJ-01' => __( 'ABOMEY', 'woocommerce' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Those state names doesn't seems to match the list: https://github.com/unicode-org/cldr/blob/master/common/subdivisions/en.xml#L522-L533

Also it's all on uppercase, it need to be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i'll apply the changes and commit it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have applied changes as mentionned, does it match now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@claudiosanches claudiosanches added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Aug 4, 2020
Copy link
Contributor

@rrennick rrennick left a comment

Choose a reason for hiding this comment

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

@manutheblacker Thanks for the help.

@rrennick rrennick added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Aug 14, 2020
i18n/states.php Outdated
'BJ-03' => __( 'Atlantique', 'woocommerce' ),
'BJ-04' => __( 'Borgou', 'woocommerce' ),
'BJ-05' => __( 'Collines', 'woocommerce' ),
'BJ-06' => __( 'Couffo', 'woocommerce' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Kouffo.

i18n/states.php Outdated
@@ -172,6 +172,20 @@
),
'BH' => array(),
'BI' => array(),
'BJ' => array( // Benin states.
'BJ-01' => __( 'Alibori', 'woocommerce' ),
'BJ-02' => __( 'Atacora', 'woocommerce' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting for second reviewer that this is the French spelling for the state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you got a point. This is the French spelling for this state because it is a French state.

Copy link
Contributor

@claudiosanches claudiosanches left a comment

Choose a reason for hiding this comment

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

Looks good, but we also need to follow the code from the CLDR.

i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
i18n/states.php Outdated Show resolved Hide resolved
Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

I approved the suggestions from Claudio to use codes from CLDR. This PR looks good to me now.

@rodrigoprimo rodrigoprimo added this to the 4.6.0 milestone Sep 12, 2020
@rodrigoprimo rodrigoprimo dismissed claudiosanches’s stale review September 12, 2020 18:54

Suggested changes were incorporated

@rodrigoprimo rodrigoprimo merged commit ce656eb into woocommerce:master Sep 12, 2020
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Sep 12, 2020
@rodrigoprimo rodrigoprimo added status: approved and removed status: needs review release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants