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

Add config options for themes #91

Closed
bradfrost opened this issue Oct 18, 2017 · 14 comments
Closed

Add config options for themes #91

bradfrost opened this issue Oct 18, 2017 · 14 comments
Assignees

Comments

@bradfrost
Copy link
Member

bradfrost commented Oct 18, 2017

Right now we have the following theme options that are ready to ship. We need to add config options for the following options that get appended to PL's pl-c-body:

  • pl-c-body--theme-light - this is the light theme, which sets PL's header and modal to a white background with dark gray text.
  • pl-c-body--theme-density-cozy increases PL's header font size and padding so PL things are a bit more prominent
  • pl-c-body--theme-density-comfortable increases PL's header padding even more to be even more prominent than the cozy density
  • pl-c-body--theme-sidebar moves PL's header and displays it as a left sidebar.

Here's what these options could look like inside of patternlab-config.json, with the default (aka current) values displayed first:

{
  "theme" : {
    "color" : "dark" | "light",
    "density" : "compact" | "cozy" | "comfortable",
    "layout" : "horizontal" | "vertical"
  }
}

I know which classes need applied, but I don't know how to wire up the config and write the conditionals inside of src/html/partials/index.html. Could use some help, @bmuenzenmeyer or @EvanLovely.

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Oct 18, 2017

hmm, yeah since that isnt a template at all (rather, just plain HTML), we may need to pursue a different approach or refactor this a bit

will think on it - i know how we could do it in short order, as long as you are okay with a potentially momentary flash of the default ui ?

@bmuenzenmeyer
Copy link
Member

bmuenzenmeyer commented Oct 19, 2017

candidates for this code would be styleguide.js or layout.js. both should have access to the provided config

@bradfrost
Copy link
Member Author

candidates for this code would be styleguide.js or layout.js. both should have access to the provided config

Are you saying the code should live in one of those two files? I didn't want to tackle a giant refactoring of JS, but like the SCSS file structure, the JS could afford to be chunked out into more modular chunks. I'd be curious to hear if you also think that's a good idea, but personally I would make a new themes.js file or something that handles the functions. styleguides.js should be about 5 different files :)

@bmuenzenmeyer
Copy link
Member

I have no shortterm desire to further refactor the frontend javascript or template engines. I think it amounts bikeshedding, and may only be necessary when we look into the JSON schema feature, if at all.

I am saying that the easiest path forward, avoiding major refactor (since the index.html is not a template that gets compiled with data), would be to have the existing javascript code read from the exposed theme config and apply classes to the PL body as needed. I was commenting mostly for my own purposes.

something like ... (enter shitty pseudocode)

//define map of key to actual css class
var themeColorValues = {
 'light': 'pl-c-body--theme-light'
}

//assign value based on mapping (if found)
var colorClass = '';
if (config && config.theme && config.theme.color) {
   colorClass = themeColorValues[config.theme.color];
}

//apply class to body
document.querySelector('.pl-js-iframe').classList.add(colorClass);
  • the same logic would apply for density and layout
  • this should work, but may result in a paint or two of the default behavior. We would have to experiment. I can hack something together...

@bmuenzenmeyer
Copy link
Member

the JS could afford to be chunked out into more modular chunks. I'd be curious to hear if you also think that's a good idea, but personally I would make a new themes.js file or something that handles the functions. styleguides.js should be about 5 different files

I should read that more closely too. sorry. we can definitely place the new logic in a new file. the build process should concatenate it all together anyways. I think the larger thing to figure out is whether it will be fast enough to avoid an ugly UX. If there is a problem, we will to get more creative.

@bradfrost
Copy link
Member Author

I should read that more closely too. sorry. we can definitely place the new logic in a new file. the build process should concatenate it all together anyways

Sounds good. We're on the same page.

I think the larger thing to figure out is whether it will be fast enough to avoid an ugly UX. If there is a problem, we will to get more creative.

Yeah, I guess my server-side rendering brain immediately went to Mustache hooks for the theme stuff to avoid flashes of default theme before config options get loaded in. Of course, I have no idea what the level of effort is to achieve that. I will say this is relevant to other not-too-distant things like user-defined PL CSS, user-defined logos, and other things. Do we want that stuff to be baked into the HTML, or added with JS?

bmuenzenmeyer added a commit that referenced this issue Oct 19, 2017
@bmuenzenmeyer
Copy link
Member

@bradfrost check out that branch. not perfect. but something!

@EvanLovely
Copy link
Member

OK, config.theme is exposed to use on the PHP side.

@EvanLovely
Copy link
Member

I've also got a PR up for implementing this: #93

@bmuenzenmeyer Does yours fire before render or after? As in: do you see the UI move around?

@EvanLovely
Copy link
Member

@bradfrost Re:

Do we want that stuff to be baked into the HTML, or added with JS?

It's gotta be JS as the config is in JS and that HTML isn't a template that gets passed data from config.

@bmuenzenmeyer
Copy link
Member

@EvanLovely let's go with your PR. My js file solution caused a flash, at least in my quick test.

@bmuenzenmeyer
Copy link
Member

In the future we should try to coordinate better too :)

@EvanLovely
Copy link
Member

👍🏼 On both comments

@bmuenzenmeyer
Copy link
Member

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

No branches or pull requests

3 participants