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

Assets improved #138

Closed
wants to merge 28 commits into from
Closed

Assets improved #138

wants to merge 28 commits into from

Conversation

adrienrn
Copy link

First, thanks for the great work you've put in writing theme-bundle. It works great.

Context

Working on a project close to a CMS, built on Symfony, our clients were asking was customizing the look-and-feel of the platform to match their brand identity.

That's how I stubbled upon this bundle and it works great but allow user to upload their theme including assets (CSS, JS, images) was the main struggle I encountered.

That's why I worked on a way to make those assets available in web/ directory. This might be a solution to some opened issues like #110.

Solution

The solution is a command, similar to assets:install.

~ $ php app/console assets:themes-install --symlink web

Then you can use the asset() twig function to reference your asset as you would do with regular bundles assets.

If the theme is part of a bundle :

<link rel="stylesheet" type="text/css" href="{{ asset('themes/mybundle/mytheme/stylesheets/style.css') }}">

If the theme is inside the app/ directory :

<link rel="stylesheet" type="text/css" href="{{ asset('themes/mytheme/stylesheets/style.css') }}">

Under the hood

Every theme ships its own assets in a public/ directory.

Directory Description
public/ Assets of this themes (images, javascripts, stylesheets, fonts, etc). It mimics the Symfony assets directory structure inside each themes.
public/images/ Holds all the static images of this theme.
public/javascripts/ Holds all the Javascript files of this theme.
public/stylesheets/ Holds the CSS files of this theme.

The command makes symlinks (or hard copies) of assets into the web/themes/ directory.

It takes the liip_theme.themes configuration and makes the right symlink – if a public/ directory is found inside each theme.

If liip_theme.themes is not defined, it just scans the project, searching for themes in liip_theme.path_patterns paths.

ThemeLocator & Installer.php

Discovering themes (dynamically) is possible by calling the service liip_theme.theme_locator and ThemeLocator::discoverThemes().

Install assets of a theme is possible calling the service liip_theme.installer – that you can call after the upload of a theme and therefore dynamically install the assets of the theme (that's why I'm linking the issue #110).


What do you think ?

@lsmith77
Copy link
Contributor

@thor / @dbu can you review this?

);

$finder = new Finder();
$paths = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

as this Bundle is still compatible with PHP 5.3 please do not use short array syntax.

@adrienrn
Copy link
Author

Made some improvements based on your comments.

cc @Morgan-pepe

@lsmith77
Copy link
Contributor

lsmith77 commented Dec 9, 2015

there are still 2 failing tests.

@adrienrn
Copy link
Author

Travis CI tells me this :

1) Liip\ThemeBundle\Tests\DependencyInjection\LiipThemeExtensionTest::testLoadWithCookie
Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException: You have requested a non-existent parameter "kernel.root_dir".

From LiipThemeBundle/Tests/DependencyInjection/LiipThemeExtensionTest.php:59

If I understood well, following the stacktrace, i added to ThemeLocator to ActiveTheme (injected in controller) and ThemeLocator needs %kernel.root_dir%.

So when ThemeRequestListener pulls ActiveTheme, it also pulls ThemeLocator (to discover themes if liip_theme.themes is not defined).

@lsmith77 Do you have some input ? I fail to see what's wrong or what to fix to pass the LiipThemeExtensionTest.

@KoriSeng
Copy link

KoriSeng commented Dec 9, 2017

Actually for this asset improvement thing this would work?

#144 (comment)

@adrienrn
Copy link
Author

I actually developed my own theming bundle since last year. I'll see if we can open-source it, in the meantime, I close this pull request.

@adrienrn adrienrn closed this Dec 10, 2017
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.

5 participants