-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Site editor: use constants rather than hard coded template strings (round 3) #54705
Conversation
Size Change: +118 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
const entityLabel = entityLabels[ entityType ] || entityLabels.wp_template; | ||
const entityLabel = | ||
POST_TYPE_LABELS[ entityType ] || | ||
POST_TYPE_LABELS[ TEMPLATE_POST_TYPE ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a very subtle change here — we can now match against TEMPLATE_PART_POST_TYPE
to get the Template part
label, which wasn't possible before. This seems like a good change to me 👍
Trunk | This PR |
---|---|
I noticed one of the Playwright tests was failing. I've just kicked it off again, but if it's still failing, perhaps due to this label being updated?
This is the line where I think it might need to be updated to Template part
?
'role=region[name="Editor settings"i] >> role=button[name="Template"i]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tracking it down! I just pushed and walked away :D
It makes me realize how many variations of Template part, template part and Template Part we have across the codebase.
I've updated to match the constant for now (Template Part). This was what I gleaned from https://make.wordpress.org/marketing/handbook/resources/style-guide-and-brand-book/#capitalizing-wordpress-terms.
I can make a note to unify them once we decide on what it should be 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes me realize how many variations of Template part, template part and Template Part we have across the codebase.
Oh, good point! At least this e2e test is case insensitive so it shouldn't break when we unify them further down the track 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related conversation in #51744
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thanks for your "constant" vigilance in improving the code quality here 😄
All testing nicely for me, LGTM! ✨
The quality is "variable", but I appreciate your support |
…ound 3) (#54705) * Another round of replacing hard-coded strings with available constants * Update E2E test selector match the label
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: cd358e4 |
What?
This is a follow up to:
Why?
To centralize names for post types and so on. Ease of management. Giving search and replace a well-deserved rest.
How?
Replacing the following strings with constants
NAVIGATION_POST_TYPE
TEMPLATE_POST_TYPE
TEMPLATE_PART_POST_TYPE
PATTERN_TYPES.user
Testing Instructions