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

Load CSS files / Specify theme path in config / coding standards #11

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mherchel
Copy link

@mherchel mherchel commented Mar 14, 2019

This PR:

  • Adds ability to load in the theme's CSS (in addition to JS)
  • Detects theme path from Pattern Lab's config file, and passes that into the file paths.
  • A couple coding standard issues (e.g. use snake_case instead of camelCase).
  • Documentation.

@waako
Copy link

waako commented Mar 15, 2019

I like the approach of using $library_tags for both CSS and JS, and the documentation (the thing so often forgotten, especially by me), it feels much cleaner than my approach in PR #8.

However, the $css_groups = $yaml_output[$file]['css']; and probably also the $js_files = $yaml_output[$file]['js']; (just for resilience) need to be wrapped in a if (isset($yaml_output[$file]['css'])) { otherwise it fails to attach the files with lots of Undefined index – as I discovered creating my PR and on testing this one.

The $path_to_theme is a nice touch, but not sure of the use case, especially after PR #10 reinforces that a relative path seems to work in all circumstances.
I appreciate it makes the path agnostic from the script and it means can just set it to ../../../.. in config.yml.

But provided the relative path works for all known use cases it may be simpler to stick to that rather than add the overhead of editing config.yml before attach_library can work in PatternLab.

@hawkeyetwolf
Copy link

@waako, are you testing with a local node server or with a Drupal instance? We (@mherchel and I) typically access PatternLab via the same server that's serving Drupal, so we need to prepend the "theme path" to make the sources match up. But if we were using a local node server with a webroot that serves directly from the theme directory, then I could see why you wouldn't need to prepend the theme path. I've got a the changes prepared to make that setting optional, just wanted to check with you before pushing.

I could be wrong, but I don't think #2 or #10 would do the trick, as we can't rely on directory names or a specific number of directories. For example, themes can live in the themes directory at the webroot or in one of the sites directories. And themes are sometimes further separated into custom and contrib directories.

@hawkeyetwolf
Copy link

On further investigation, I realized my folly: A webserver serving directly from the patternlab directory can't access files above its webroot, so it could never serve the library files! So that must not be what you mean. And we only need to worry about accessing the patternlab output via the same webserver as Drupal.

I can't see any way to make it work reliably without the config variable. Is there something I'm missing?

@waako
Copy link

waako commented Jul 26, 2019

I guess it might depend on your setup, it's true that our theme is based off Emulsify, same for #10 so the paths depths and folder structure may be very similar, .e.g compiled css/js is in dest folder at root of theme.
It depends on path you set for library items of course, but if they are set from theme root, then should be fine.

It's about the relative path from the generated HTML files and the static assets, regardless of how you access them.
We access our pattern-lab either through localhost:3000/pattern-lab/public or domain.com/themes/custom/themename/pattern-lab/public and using relative path ../../../../ works in both cases for us.

Sorry if not making sense, late Friday night and about to go on holiday so wanted to get a reply in.
But basically if you're pattern-lab generated HTML files have similar structure to described here #10 (comment) you only need that relative path

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