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

Support menu text attribute #3

Merged
merged 1 commit into from
Sep 28, 2022
Merged

Conversation

alwaysblank
Copy link
Contributor

This adds conditional support for defining a "menu title," i.e. a different title for the heading that will be used in other locations (i.e.e a ToC or the local nav widget).

@CoreyBenefiel
Copy link

Hey @alwaysblank Can this be merged into staging so I can test https://app.zenhub.com/workspaces/hm-playbook-607442e6aab6bf000ecd201d/issues/humanmade/hm-playbook-theme/472? Anything holding this up?

@alwaysblank
Copy link
Contributor Author

@CoreyBenefiel This wasn't merged in because of the following:

  • This plugin doesn't actually have anything in the main branch; I don't know why. There doesn't seem to be documentation in the plugin. @gonzomir may know more since he built it. In any event, I was reluctant to merge to main without knowing more.
  • I don't know if anything else is using this plugin and if modifying the code will cascade out to other sites unexpectedly (I don't imagine so, but I didn't have time to check).
  • This PR (and the other PR I filed, which would also need to be merged) haven't been reviewed.

In my opinion I think you could probably merge all the PRs on this repo and things will be fine, but I wouldn't recommend doing some without having an engineer look at the points mentioned above first.

@gonzomir
Copy link
Collaborator

gonzomir commented Aug 4, 2022

Hi, @CoreyBenefiel and @alwaysblank, as far as I remember, the plugin needed to be tested and confirmed it does the job for the new Playbook design. Then the initial PR #1 should have been reviewed and merged, but that never happened 🤷‍♂️. Since the plugin is not used anywhere else, I think it's save to merge everything into main after passing code review.

@MiguelAxcar MiguelAxcar changed the base branch from main to allow-original-ids September 27, 2022 01:02
@MiguelAxcar
Copy link
Contributor

The pointer of this repo was set to the branch allow-original-ids (@kadamwhite was working on it 3 days ago) instead of main, and changing base branch of this PR to allow-original-ids there is only a small change to be added.

The change that this PR includes affects only using a menu title if editor specified it.

if ( preg_match( '/data-menu-title="([^"]*)"/', $item->html, $menu_text_matches ) ) {
	$item->title = $menu_text_matches[1] ?: $item->title;
}

https://github.com/humanmade/hm-table-of-contents-plugin/pull/3/files

I tried testing this feature to see if it does the job. Could not find it in a walk-through and could not also find any information about how to test it.

@CoreyBenefiel could you describe how can I test the hm-toc plugin, and also, what this change of using a menu title if editor specified it is expected to do? As soon as I check that it's working, I can proceed merging it. Thanks!

@alwaysblank
Copy link
Contributor Author

@MiguelAxcarHM Did you look at the relevant PR on the playbook theme repo? https://github.com/humanmade/hm-playbook-theme/pull/472 Without that merged, this PR won't do anything.

@alwaysblank alwaysblank changed the base branch from allow-original-ids to main September 28, 2022 01:32
Copy link
Contributor

@MiguelAxcar MiguelAxcar left a comment

Choose a reason for hiding this comment

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

It works as expected.

@MiguelAxcar MiguelAxcar merged commit 9fc7d40 into main Sep 28, 2022
@MiguelAxcar MiguelAxcar deleted the support-menu-text-attribute branch September 28, 2022 03:06
@CoreyBenefiel
Copy link

Absolutely smashed this one out of the park @alwaysblank @MiguelAxcarHM. I expected I would need to manually remove the content menu block from individual pages over time, but I can see this feature not only works for new content but also works for legacy content AND deprecates the block. Awesome! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants