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

Add new filter to change the port file path #1

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@igmoweb
Contributor

igmoweb commented Nov 14, 2017

The code assumes that we're always working on a theme but we could be using a plugin. This is an actual case for The Sun.

@igmoweb igmoweb requested a review from rmccue Nov 14, 2017

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 14, 2017

Member

Could we make the root directory a parameter to enqueue_assets() instead? e.g. enqueue_assets( __DIR__, 'theme-name', [ 'maybe', 'deps' ] )

Member

rmccue commented Nov 14, 2017

Could we make the root directory a parameter to enqueue_assets() instead? e.g. enqueue_assets( __DIR__, 'theme-name', [ 'maybe', 'deps' ] )

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 14, 2017

Member

Actually, better idea, let's redesign the interface to this:

<?php
/**
 * @param string $directory Root directory containing `src` directory.
 * @param array $opts {
 *     @type string $handle  Style/script handle. (Default is last part of directory name.)
 *     @type array  $scripts Script dependencies.
 *     @type array  $styles  Style dependencies.
 * }
 */
function enqueue_assets( $directory, $opts = [] );
Member

rmccue commented Nov 14, 2017

Actually, better idea, let's redesign the interface to this:

<?php
/**
 * @param string $directory Root directory containing `src` directory.
 * @param array $opts {
 *     @type string $handle  Style/script handle. (Default is last part of directory name.)
 *     @type array  $scripts Script dependencies.
 *     @type array  $styles  Style dependencies.
 * }
 */
function enqueue_assets( $directory, $opts = [] );
@igmoweb

This comment has been minimized.

Show comment
Hide comment
@igmoweb

igmoweb Nov 14, 2017

Contributor

@rmccue enqueue_assets is not enqueuing any style. Do we want to do that? When Webpack is up, we don't need to enqueue styles at all, they are inserted inline

Contributor

igmoweb commented Nov 14, 2017

@rmccue enqueue_assets is not enqueuing any style. Do we want to do that? When Webpack is up, we don't need to enqueue styles at all, they are inserted inline

@igmoweb

This comment has been minimized.

Show comment
Hide comment
@igmoweb

igmoweb Nov 14, 2017

Contributor

@rmccue I pushed a few changes:

  1. I added the interface you said but also an extra base_url param for production builds.
  2. The JS and CSS filenames are now read from assets-manifest.json (for production)
Contributor

igmoweb commented Nov 14, 2017

@rmccue I pushed a few changes:

  1. I added the interface you said but also an extra base_url param for production builds.
  2. The JS and CSS filenames are now read from assets-manifest.json (for production)

@igmoweb igmoweb referenced this pull request Nov 14, 2017

Closed

Enqueue built CSS #2

@igmoweb

This comment has been minimized.

Show comment
Hide comment
@igmoweb

igmoweb Nov 24, 2017

Contributor

@rmccue We've been testing this branch for a few weeks and it's working fine for both, dev and build environments. Do we want to do anything else here?

Contributor

igmoweb commented Nov 24, 2017

@rmccue We've been testing this branch for a few weeks and it's working fine for both, dev and build environments. Do we want to do anything else here?

@kadamwhite

This comment has been minimized.

Show comment
Hide comment
@kadamwhite

kadamwhite Nov 29, 2017

Collaborator

@igmoweb I incorporated much of this PR into #6 which just landed, could you test out with the latest and let me know if that one supersedes this?

Collaborator

kadamwhite commented Nov 29, 2017

@igmoweb I incorporated much of this PR into #6 which just landed, could you test out with the latest and let me know if that one supersedes this?

@igmoweb

This comment has been minimized.

Show comment
Hide comment
@igmoweb

igmoweb Nov 29, 2017

Contributor

Excellent @kadamwhite . Closing here.

Contributor

igmoweb commented Nov 29, 2017

Excellent @kadamwhite . Closing here.

@igmoweb igmoweb closed this Nov 29, 2017

@igmoweb igmoweb deleted the new/port-file-filter branch Nov 29, 2017

@igmoweb igmoweb restored the new/port-file-filter branch Nov 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment