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

Introduce a filter to override Document label in Document Settings Header. #17311

Conversation

@desaiuditd
Copy link
Contributor

commented Sep 3, 2019

Description

I need to be able to rename the Document label in the sidebar. So that I can differentiate between different post types and custom post types as well. It could also be a generic custom label e.g., Article, Story etc. for any specific newsroom.

image

I've introduced a filter editor.sidebar.settingsHeader.documentLabel for the document label which defaults to Document and then any downstream consumer can update this label as per needed.

How has this been tested?

Tested this on my local WordPress instance by running below snippet from a custom plugin.

const {
	hooks: { addFilter },
	i18n: { __ }
} = wp;

addFilter(
	'editor.sidebar.settingsHeader.documentLabel',
	'custom/block-editor/override-document-settings-header-label',
	() => __( 'Story', 'custom' )
);

Screenshots

Result after running above code:

image

Types of changes

  • New JS filter editor.sidebar.settingsHeader.documentLabel for the document label which is hard code for now.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@desaiuditd desaiuditd requested a review from talldan as a code owner Sep 3, 2019

@@ -11,11 +12,12 @@ import SidebarHeader from '../sidebar-header';

const SettingsHeader = ( { openDocumentSettings, openBlockSettings, sidebarName } ) => {
const blockLabel = __( 'Block' );
const documentLabel = applyFilters( 'editor.sidebar.settingsHeader.documentLabel', __( 'Document' ) );

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Sep 3, 2019

Member

Hi @gziolo, any thoughts related to this new filter?

This comment has been minimized.

Copy link
@desaiuditd

desaiuditd Sep 4, 2019

Author Contributor

I could also use some help in the naming of this filter, if this is the right approach moving forward.

This comment has been minimized.

Copy link
@gziolo

gziolo Sep 4, 2019

Member

I shared my thoughts here: #17311 (review). I marked PR as request changes only to ensure that we have a proper discussion about customizing UI labels with some guidelines about implementation documented before we land this change.

@@ -11,11 +12,12 @@ import SidebarHeader from '../sidebar-header';

const SettingsHeader = ( { openDocumentSettings, openBlockSettings, sidebarName } ) => {
const blockLabel = __( 'Block' );
// translators: ARIA label for the Document sidebar tab, not selected.
const documentLabel = applyFilters( 'editor.sidebar.settingsHeader.documentLabel', __( 'Document' ) );

This comment has been minimized.

Copy link
@desaiuditd

desaiuditd Sep 4, 2019

Author Contributor

Also, I did not see any existing pattern to document applyFilters.

E.g., in PHP, we document the apply_filters wherever they are introduced along with the parameters and return values.

Do we have such pattern to add documentation for JS filters/hooks?

Block Filters page does not seem like a good place for these extra utility hooks, as they are not directly related to blocks.

@ffxluca
ffxluca approved these changes Sep 4, 2019
Copy link

left a comment

👏

@gziolo
Copy link
Member

left a comment

Thank you for opening this pull request.

To be clear. I understand the need to have more ways to customize the user interface. At the same time, I would like to notice that we don't use JS filters for UI labels in any place in the block editor. There are a few labels which are filterable using PHP filters, like the Paragraph block placeholder which is passed to the editor through its settings:

const { bodyPlaceholder } = getSettings();

Related PHP filter:
https://github.com/WordPress/wordpress-develop/blob/00b03f2a6f2c21ebf9975c9c08976358681400ba/src/wp-admin/edit-form-blocks.php#L270

This should be discussed first, what's the best strategy moving forward with adding new filters for UI labels and what naming conventions should be used before we proceed with this particular use case.

@gziolo gziolo requested review from mtias and youknowriad Sep 4, 2019

@desaiuditd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

I also thought this to be an editor level setting first, but I felt it's not affecting the whole editor and only limited to the sidebar section. Also, I could not find any single place where all the labels are listed as a source of truth. So ended up adding the filter in the component itself.

But if this is useful in wider sense, then taking a generic approach and a single collection of all the labels at one place makes more sense.

My next question would be, which labels are good candidates to be part of this collection? Not everyone would like to customise them. So should it be part of WordPress Core or the Gutenberg plugin itself?

@aprea

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

This should be discussed first, what's the best strategy moving forward with adding new filters for UI labels and what naming conventions should be used before we proceed with this particular use case.

@gziolo where is the best place to kick off this discussion? In this PR?, the #core-editor channel, elsewhere?

@swissspidy

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@aprea @gziolo We could discuss this in tomorrow's core editor meeting on Slack

@swissspidy

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Reminds me a bit of #12517 by the way :-)

@desaiuditd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Reminds me a bit of #12517 by the way :-)

Wow, that's wild. I would not have imagined that. 😃

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

We could discuss this in tomorrow's core editor meeting on Slack

Sure, it sounds like a great topic to discuss.

Reminds me a bit of #12517 by the way :-)

I was thinking about it the other day. It's definitely one of the options as long as we ensure it performs well enough so you don't have to apply the same filters all over again on every component's re-render.

I was also thinking about the alternatives we could investigate. I came up with one idea so far. Introduce useTranslation React hook which you would have to explicitly call with some unique name for the filter to open a given string for modifications. The rationale behind it is, that sometimes you use the same translation string in many places and you don't necessarily want to update all of them at the same time. Having React hook gives us also more flexibility as we could re-render component when a new filter is applied for the translation, even after the page was rendered.

Well, we could also discuss some set of constraints which would make the initial implementation safer if we do something like #12517. Example:

  • all filters need to be applied before the initial render
  • only translations with context applied can be filterable
@desaiuditd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Thoughts on this from last Weekly Editor Chat:

@gziolo:

We still would want to use applyFilters, yes
We were concerned in the past about performance implications and now I think that another issues is the discovery of all filterable translations

@youknowriad:

One "somehow related" idea I was thinking about is the fact that applyFilters is very sensitive to the scripts loading order and we have an opportunity to build a useFilter alternative to fix this.

@desaiuditd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

@youknowriad, if I'm to take your approach of introducing new useFilters hook, in which package, would you put that?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

If we were to add a useFilter it should probably go in the filters package. I guess this means adding a @wordpress/element dependency to that package but maybe it's ok? Thoughts?

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

If we were to add a useFilter it should probably go in the filters package. I guess this means adding a @wordpress/element dependency to that package but maybe it's ok? Thoughts?

I was thinking, we might want to place it inside @wordpress/plugins which depends on both @wordpress/element and @wordpress/hooks already.

@desaiuditd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

Oh. A new package of its own?

adding a @wordpress/element dependency

This should be fine, I guess.

But, what qualifies for a new package? What purpose it's going to serve? What else it's going to contain?

@wordpress/plugins

This seems fine. I would not have much of an opinion on this (Lack of knowledge). Would defer to others.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

I was actually thinking about the hooks package (my brain said filters) :)

@gziolo

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

I was actually thinking about the hooks package (my brain said filters) :)

It should work as well. Having useFilter hook inside @wordpress/hooks might be an interesting challenge in terms of explaining the difference between WordPress and React hooks :) However, this is what it is regardless. Just an observation. I think @youknowriad is right.

@adamsilverstein - any thoughts on this topic as you are expert in terms of WordPress hooks :)

@desaiuditd

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

@wordpress/hooks seems to be a plain JavaScript package. Also, there's a general vibe that @wordpress/hooks can be used in any other system which may not necessarily use React. So do we have to maintain that backward compatibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.