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

Change subhead to say subheading #6407

Merged
merged 2 commits into from
May 15, 2018
Merged

Change subhead to say subheading #6407

merged 2 commits into from
May 15, 2018

Conversation

karmatosed
Copy link
Member

Try branch for #5790.
Props @afercia

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I think we should also update block's name, file name and everything that uses the old name. Not sure if core/subhead is an easy change in terms of backward compatibility.

@karmatosed
Copy link
Member Author

@gziolo ah ok I didn't go deeper than copy as wasn't sure. Let me dive more! Thanks.

@karmatosed
Copy link
Member Author

Not sure if core/subhead is an easy change in terms of backward compatibility.

I think this isn't and I found with changing things too much things imploded a little. What would be the basics I should change as a 'worst case'?

@gziolo gziolo added the [Feature] Blocks Overall functionality of blocks label Apr 30, 2018
@gziolo gziolo added this to the 2.8 milestone Apr 30, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I asked @youknowriad if there is an easy way to update core/subhead to core/subheading. He suggested using similar solution to:
https://github.com/WordPress/gutenberg/blob/master/blocks/api/parser.js#L231-L234

However, it doesn't work with Subheading because it uses auto-generated className for the block and it breaks all the existing blocks.

I would limit this change to what we already included.

@aduth
Copy link
Member

aduth commented Apr 30, 2018

I would limit this change to what we already included.

Would that mean changing the file name back to subhead/ if we're keeping with the core/subhead technical name?

@gziolo
Copy link
Member

gziolo commented Apr 30, 2018

@aduth I wanted to leave it as it is, but if you think we should revert this particular change, then let’s do it. Both aren’t perfect so I’d follow your judgement here.

@karmatosed
Copy link
Member Author

Just looping back but some feedback on this is that it may be preferred to be called subtitle. Before we change things here we should make sure there's a decision in that issue #5790

@aduth
Copy link
Member

aduth commented Apr 30, 2018

I think the slug and the folder name should match. Ideally the slug and the canonical "title" align, though there's already a few cases of precedent where we've been fine to diverge: namely the "Classic" block (core/freeform) and "Shared Block" block (core/block). In both of these cases, the directories still align with the actual slug, regardless of its human-readable named title.

@gziolo gziolo force-pushed the try/rename-subhead-block branch from 7e59a04 to 3cde112 Compare May 2, 2018 09:11
@gziolo
Copy link
Member

gziolo commented May 2, 2018

@karmatosed @aduth - this PR was updated to contain only text changes as discussed. As soon as it's decided what name this block should have, those texts should be updated. We can also add the old name as key in block's definition to improve discoverability.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code-wise 👍

@aduth aduth modified the milestones: 2.8, 2.9 May 3, 2018
@aduth
Copy link
Member

aduth commented May 3, 2018

Punting to 2.9 since discussion in #5790 (comment) appears unsettled.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label May 10, 2018
@gziolo gziolo force-pushed the try/rename-subhead-block branch 2 times, most recently from 2632519 to 862d905 Compare May 10, 2018 10:47
@gziolo gziolo force-pushed the try/rename-subhead-block branch from 862d905 to 32efa02 Compare May 15, 2018 15:52
@gziolo
Copy link
Member

gziolo commented May 15, 2018

Because of keyword issues on headings where subtitle is a keyword already, for now lets go with subheading and close this. It is a fairly easy change to iterate on.

Subheading it is 👍

@gziolo gziolo merged commit 787b8d5 into master May 15, 2018
@gziolo gziolo deleted the try/rename-subhead-block branch May 15, 2018 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Decision Needs a decision to be actionable or relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants