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

🍕 Fix pre-publish site controller reference #713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjpaulino
Copy link
Member

This function is not being executed in our clay instance because it is not being picked up and assigned to the site's controller. It's a function to add metadata to a page's meta.

@jjpaulino jjpaulino self-assigned this Aug 1, 2024
Copy link
Member

@james-owen james-owen left a comment

Choose a reason for hiding this comment

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

Nice catch.

Comment on lines 177 to +180
_.assign(site, _.pick(controller, 'resolvePublishing'));
_.assign(site, _.pick(controller, 'resolvePublishUrl'));
_.assign(site, _.pick(controller, 'modifyPublishedData'));
_.assign(site, _.pick(controller, 'assignToMetaOnPublish'));
Copy link
Contributor

@macgyver macgyver Aug 2, 2024

Choose a reason for hiding this comment

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

i know this is just like.. the pattern set by whoever came before, but wouldn't this be written more simply as

    _.assign(site, {
      providers,
      // things to remember from controller
      ..._.pick(controller, 'resolvePublishing', 'resolvePublishUrl', 'modifyPublishedData', 'assignToMetaOnPublish')
    });

or like.. w/o lodash...

    Object.assign(site, {
      providers,
      // I don't know why it's important that we not just assign everything from the controller
      resolvePublishing: controller.resolvePublishing,
      resolvePublishUrl: controller.resolvePublishUrl,
      modifyPublishedData: controller.modifyPublishedData,
      assignToMetaOnPublish: controller.assignToMetaOnPublish
    });

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.

3 participants