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 #2812 — support ignoring assets in themes #2813

Merged
merged 1 commit into from Jun 3, 2017
Merged

Conversation

@Kwpolska
Copy link
Member

Kwpolska commented Jun 2, 2017

This is #2812. cc @gwax (regarding making base smaller)

This implements the feature, without adding any ignores just yet. (baguettebox might be the first)

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska Kwpolska added this to the v7.8.7 milestone Jun 2, 2017
@Kwpolska Kwpolska requested a review from ralsina Jun 2, 2017
@gwax
Copy link
Contributor

gwax commented Jun 2, 2017

Ignoring individual assets interacts poorly with changes to an underlying theme; say a new asset is added to base. If the person adding the asset forgets to ignore it in a child theme, everyone that uses the child theme will start receiving the asset.

Of course, it's also potentially problematic if they don't. What happens if base adds an asset and requires it in a template. What if the child doesn't want the asset but hasn't overridden the template.

@gwax
Copy link
Contributor

gwax commented Jun 2, 2017

Fundamentally, we're fighting against a fragile base class problem.

@ralsina
Copy link
Member

ralsina commented Jun 2, 2017

@gwax

If base adds a new asset, and the child theme doesn't need it, it's on the child theme author to fix it, or some random asset gets published and 50kb get wasted.

I suppose we could just add a ** blacklist, where the child theme just says "give me no assets" and everything is manual from then on.

And yes, the other problem also exists, so changing the base theme can break the child in that case. OTOH, if the child theme blocks assets, then it's even more the author's task to handle that.

Honestly, I would block nothing and live with a few random extra assets not linked to anything in the output. Seems to me like a really minor thing to worry about :-)

@ralsina
ralsina approved these changes Jun 2, 2017
Copy link
Member

ralsina left a comment

+1

@Kwpolska Kwpolska merged commit f45bafb into master Jun 3, 2017
3 of 5 checks passed
3 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Kwpolska Kwpolska deleted the ignoring_assets branch Jun 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.