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

Expanding on sentence case to everywhere else #18758

Merged
merged 13 commits into from
Dec 16, 2019
Merged

Conversation

karmatosed
Copy link
Member

So far we have only done this in the tooltips and in menus. If sentance case of right approach we should consider taking this across all areas.

Included in this:

  • Side panel
  • Block library
  • Settings

There may be areas missed but gives a starting view to see if everyone agrees to go forward with this across everything. If we do, then we need a patch for outside the editor.

Expands on #16764

So far we have only done this in the tooltips and in menus. If sentance case of right approach we should consider taking this across all areas.

Included in this:

- Side panel
- Block library
- Settings

There may be areas missed, but gives a starting view to see if everyone agrees to go forward with this across everything. If we do, then we need a patch for core.

Expands on #16764
@karmatosed
Copy link
Member Author

karmatosed commented Nov 26, 2019

Some images with these changes:

image

image

image

@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Nov 26, 2019
@enriquesanchez
Copy link
Contributor

I agree this is much better and happy to see the work that started on #16764 expand to other areas.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Nov 26, 2019
@gziolo gziolo added [Type] Copy Issues or PRs that need copy editing assistance [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement. and removed [Type] Code Quality Issues or PRs that relate to code quality labels Nov 27, 2019
@karmatosed
Copy link
Member Author

@noisysocks I don't know if you can help here but the tests seem stuck again :(

@mkaz
Copy link
Member

mkaz commented Dec 9, 2019

@karmatosed This time they are valid tests fails. The renaming of the default categories "Common blocks", "Layout elements", and "Reusable blocks" is causing some inserter menu test to fail.

Tests at: packages/block-editor/src/components/inserter/test/menu.js

Likewise with the description in this test:
FAIL packages/edit-post/src/components/keyboard-shortcut-help-modal/test/index.js

And label in this test:
FAIL packages/edit-post/src/components/options-modal/test/index.js

You can see the error messages here: https://travis-ci.com/WordPress/gutenberg/jobs/260658429

Fixes test issues.
@@ -149,6 +149,14 @@ function gutenberg_override_translation_file( $file, $handle ) {
}
add_filter( 'load_script_translation_file', 'gutenberg_override_translation_file', 10, 2 );

function gutenberg_override_posttype_labels( $labels ) {
$labels->featured_image = 'Feaaatured image';
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the macbook keyboard is not working properly :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops :D

@mcsf mcsf force-pushed the try/sentance-everywhere branch from c81a72f to c31ce37 Compare December 11, 2019 14:01
… to conform to sentence-style capitalisation across the editor UI.

(This is a quick commit to show the idea. Do expand on it and polish.)

From Slack: "If we used this it would have to be properly marked [e.g.
@todo] and commented so that before the next core release we could
revisit these labels as a whole."
@mcsf mcsf force-pushed the try/sentance-everywhere branch from c31ce37 to c7f1e89 Compare December 11, 2019 16:54
lib/client-assets.php Outdated Show resolved Hide resolved
*
* @return object Object with all the labels, including overridden ones.
*/
function gutenberg_override_posttype_labels( $labels ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this function in Gutenberg?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Soean the problem is that our labels come from core so we can't have sentence case without. I will loop @mcsf in here as they worked on this solution to help.

Copy link
Member

Choose a reason for hiding this comment

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

Then we have to change it in Core :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Soean, thanks for that watchful eye. :) Indeed, we need to change it in core.

c7f1e89 was a drive-by commit to unblock Tammie. The commit message itself did mention the need to address this in core, but commit messages aren't very visible here. This all came from this thread in Slack, which also acknowledged the need to address the root cause.

The question that this commit raised is: in the interest of landing something consistent here in #18758, can we add this filter for the plugin, provided that we also open the appropriate Trac tickets?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarification.
If a post type has a different label than "Featured image" for example "Book cover", then the filter overrides the value? This could be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, now it's translateable and it works for post und page post type.
So we can merge it and link it in the trac ticket, so we can remove it after merging it in core.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @mcsf, it seems this change needs to be backported to core. Would you be able to submit a core patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do, @jorgefilipecosta.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@karmatosed karmatosed merged commit ab8af23 into master Dec 16, 2019
@karmatosed karmatosed deleted the try/sentance-everywhere branch December 16, 2019 16:30
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@jorgefilipecosta jorgefilipecosta added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 4, 2020
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Copy Issues or PRs that need copy editing assistance [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants