[Full Site Editing]: Update descriptions for creating taxonomy templates #42191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
requested review from
mcsf,
scruffian,
mtias,
jameskoster,
jorgefilipecosta and
annezazu
|
Size Change: +202 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
annezazu
approved these changes
Jul 6, 2022
It works, but is not very elegant, and in cases where the slug matches the title it doesn't feel all that helpful 🙈 @ntsekouras can we not determine the post type that taxonomy is assigned to? Then we might do something like:
|
|
Independently of Jay's suggestion, maybe the UI could only show the extra information (slug or something else) if it detects conflicting names. |
I thought about that yes, we can do it.
We could also apply this optimization to check if the slug is the same with the label. Any change we end up applying it will need to be reflected to a created template's description which is not handled yet, as I mention in the description.
Any thoughts about this? Do we really need the
I'm not sure we should include post types in this. The problem we have is with labels so we can still end up with similar descriptions if the taxonomies are associated with the same post types. Another example would be a taxonomy from a plugin to change the post types, but created templates would have the old ones persisted in their descriptions. |
Good point, no I don't think we do need 'Single'.
I suppose this could work, assuming we only display the slug when the name is a duplicate 👍 |
ntsekouras
force-pushed
the
add/slug-to-taxonomy-templates-desciption
branch
from
6d72561 to
65165c7
Compare
|
Thanks @ntsekouras, this is definitely better now.
A thought – now that we removed 'single' I'm wondering whether we can remove 'taxonomy' as well, or is that a step too far? :D It feels a bit redundant given that the description confirms it's a taxonomy template. Personally I find this:
to be marginally more intuitive than:
Though we should probably include the slug in the description as well. What do you think? Side note: if we agree this makes sense we can probably do the same for single templates. IE "Single item: Product" could just be "Product". |
mcsf
reviewed
Jul 12, 2022
Comment on lines
-69
to
-80
| getTitle: ( labels ) => | ||
| sprintf( | ||
| // translators: %s: Name of the taxonomy e.g: "Cagegory". | ||
| __( 'Single taxonomy: %s' ), | ||
| labels.singular_name | ||
| ), | ||
| getDescription: ( labels ) => | ||
| sprintf( | ||
| // translators: %s: Name of the taxonomy e.g: "Product Categories". | ||
| __( 'Displays a single taxonomy: %s.' ), | ||
| labels.singular_name | ||
| ), |
There was a problem hiding this comment.
Because the category and tag that also share this config are part of the default templates and we'll always override them from server side.
Comment on lines
+122
to
+124
| __( 'Taxonomy: %1$s %2$s' ), | ||
| labels.singular_name, | ||
| _needsUniqueIdentifier ? `(${ slug })` : '' |
There was a problem hiding this comment.
Should probably leave the punctuation up to translators:
Suggested change
| __( 'Taxonomy: %1$s %2$s' ), | |
| labels.singular_name, | |
| _needsUniqueIdentifier ? `(${ slug })` : '' | |
| __( 'Taxonomy: %1$s (%2$s)' ), | |
| labels.singular_name, | |
| slug |
Which of course means moving the ternary out of the sprintf call, and instead have two separate sprintf calls.
Comment on lines
+195
to
+204
| taxonomyLabels = publicTaxonomies?.reduce( | ||
| ( accumulator, { labels } ) => { | ||
| const singularName = labels.singular_name.toLowerCase(); | ||
| accumulator[ singularName ] = | ||
| ( accumulator[ singularName ] || 0 ) + 1; | ||
| return accumulator; | ||
| }, | ||
| {} | ||
| ); | ||
| return publicTaxonomies; |
There was a problem hiding this comment.
We both know that in practice this "works", but it's a huge code smell.
Similarly to how async code can only be called by other async code¹, in Gutenberg we have a pure context and a "tainted" component-bound context, where React hooks must live. It's clear here that we have an increasing clash between those two contexts, with pieces that started out pure, like the entity configs, clashing with most of the dynamic data "trapped" in the impure hook-based context.
I think we need to make a decision on the architecture of the logic for adding templates. Some stream-of-consciousness ideas (take them with a grain of salt):
- Keep
needsUniqueIdentifierpure but add a third argument namednamingConflictswhose values are pre-computed nearuseExtraTemplates. - Rearchitect away from this system of nested hooks and towards something more state-based (e.g. a large pure state selector) that can purely consume dynamic data from the core store.
- Rearchitect away from this system of pure per-entity configs and hardcode entity rules inside a more monolithic set of hooks.
- Leverage React context somehow to share the logic and pre-computations across
new-templateandadd-custom-template-modal
¹: And similarly to how, in other languages, monadic code (e.g. I/O monads, unsafe code blocks) can't be handled in "pure" contexts
Comment on lines
+46
to
+54
| const _needsUniqueIdentifier = needsUniqueIdentifier( labels, slug ); | ||
| const augmentedTitle = _needsUniqueIdentifier | ||
| ? sprintf( | ||
| // translators: Represents the title of a user's custom template in the Site Editor, where %1$s is the template title and %2$s is the slug of the taxonomy, e.g. "Category: shoes (product_tag)" | ||
| __( '%1$s %2$s' ), | ||
| title, | ||
| _needsUniqueIdentifier ? `(${ slug })` : '' | ||
| ) | ||
| : title; |
There was a problem hiding this comment.
Logically unnecessary ternary:
_needsUniqueIdentifier ? `(${ slug })` : ''Nit: I suggest declaring title with let and doing something like:
Suggested change
| const _needsUniqueIdentifier = needsUniqueIdentifier( labels, slug ); | |
| const augmentedTitle = _needsUniqueIdentifier | |
| ? sprintf( | |
| // translators: Represents the title of a user's custom template in the Site Editor, where %1$s is the template title and %2$s is the slug of the taxonomy, e.g. "Category: shoes (product_tag)" | |
| __( '%1$s %2$s' ), | |
| title, | |
| _needsUniqueIdentifier ? `(${ slug })` : '' | |
| ) | |
| : title; | |
| if ( needsUniqueIdentifier( labels, slug ) ) { | |
| title = sprintf( | |
| // translators: Represents the title of a user's custom template in the Site Editor, where %1$s is the template title and %2$s is the slug of the taxonomy, e.g. "Category: shoes (product_tag)" | |
| __( '%1$s (%2$s)' ), | |
| title, | |
| slug | |
| ); | |
| } |
This was referenced Jul 13, 2022
|
Closing in favor of #42457 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Follow up of: #41875
Addresses: #41875 (comment)
In site editor a user can create various templates and as of #41875, can create any taxonomy template. The design to create a template uses a taxonomy's
labels.The problem here is that some taxonomies(ex: Product categories from WooCommerce) have some identical labels with Category. This results to:
Categoryis the WP category andSingle taxonomy: Categoryis the Products category that happens to have the same label. We face the same situation in the create template's description.How?
The only unique identifier for taxonomies seems to be the
slug. This initial commit just adds it the menu item description in the end and nothing more intentionally. Obviously we need to decide where and how to add this information to make the most sense.An alternative could be to update the main titles from
Single taxonomy: Category->Taxonomy: Category ($slug)and not the descriptions.