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

RFC: Cascadable ("nested") themes for form styling #5604

Closed
sminnee opened this Issue May 27, 2016 · 65 comments

Comments

Projects
None yet
9 participants
@sminnee
Member

sminnee commented May 27, 2016

UPDATED 16 JUNE - theme identifiers updated

PR #5471 starts introducing bootstrap HTML for form fields. The issue that @chillu raises is that this will break everyone else's forms that rely on the old HTML. We need to find a way of providing a new set of styles for the bootstrap forms, that can be optionally applied.

One approach to this would be to have a CMS theme that contains all the theme styles, and apply that. However, that presents some difficulties:

  • Anyone else who wanted to use bootstrap would need to repeat our work
  • The CMS styles would need to be stored outside of the module, in the project's themes folder.

API changes

  • Allow the SSViewer::set_theme() to accept multiple, ordered arguments.
  • Allow the following strings to be used to reference packages:
    • <vendor>/<package>: Reference a composer package. The theme will be stored in the root of the package. Eg: "silverstripe/simple".
    • <vendor>/<package>:<theme>: Reference a "named theme" in the package, stored as a direct subfolder of the "themes/" folder within the package. Eg: "silverstripe/framework:bootstrapforms".
    • /<path>: Reference a theme contained within the subfolder of the SilverStripe project. Eg: "/mysite"

In addition, we have the old, deprecated syntax: <theme>. It will reference a "named theme" in the project, that is a theme at "BASE_PATH/themes/". Instead of of this, use the syntax "/themes/". However, a better bet is to move your templates to /mysite, if they are project-specific, or to a composer package that you reference by its fully-qualified name, if they are part of a shared theme.

Default themes

In SS3, templates provided by the mysite folder and each module are treated separately from the theme engine. With the change proposed in this RFC, these would be treated as themes in their own right, e.g. '/mysite' or 'silverstripe/blog'.

This would mean there are a lot more themes to join together.

  • SSViewer::default_themes() to return the default theme cascade (/mysite, then every module)
  • SSViewer::set_themes() to replace the theme cascade in its entirety.
  • SSViewer::add_themes() to prepend additional themes onto the current stack. If add_themes() is passed a theme already in the cascade, then the given theme is pulled to a higher precedence place in the list.

For example, SSViewer::add_themes(["/mysite", "silverstripe/simple"]) will set the site to use the simple theme but ensure that templates in /mysite override the theme.

More sophisticated controls are being left out of the initial implementation until we see more use-cases. However, this approach will make it easier to provide options that tweak the priorities of different themes in ways that would have been awkward in SS3.

Theme bundles

The current behaviour that the theme "simple_forum" will be automatically included if you have selected the "simple" theme will be dropped. Instead developers will need to use: SSViewer::add_theme(['simple','simple_forum'])

Limitations

We're not, in this change, going to write the code that handles storing themes in the /vendor folder or looking for them in arbitrary locations. So, we referenced themes must be of type silverstripe-theme or silverstripe-module, and sub-themes must existing in packages of type silverstripe-module. We will rely on our existing rules for package folder location for now, and solve that in the future.

The code will need to look for the theme '/' in either '/themes/' (look here first) or '/' (look here seocnd). The potential for name conflicts is an accepted limitation for now.

Use

With theme cascade in place, themes can be created that cater to specific needs. For example, we can create a silverstripe/framework:themes/bootstrapforms theme that will provide form styling appropriate for bootstrap. This can be included into the CMS without disrupting other use-cases.

Alternatives

Possible ideas:

  • We could hard-code module-packaged themes to be stored at themes/<subthemename>. This would make it easier to build a manifest of them. It would also make theme references shorter. However, it wouldn't be clear how to reference the content of mysite/templates as a theme.
  • We could let module provide yaml config that provided a mapping of longer theme identifiers to shorter theme names. This would be another way of building a manifest of themes, but would make the manifest generation reliant on config generation, which may be a circular dependency.

Ideas I don't like:

  • Rather than allowing arbitrary theme folders, we could either allow any modules to have a theme subfolder, or we could specify multiple THEME_DIR values. This strikes me as a bit inflexible, and not substantially beneficial. It may be a simpler starting point, if we wanted to start with a smaller change.
  • A construct other than themes for form styles. However, I don't think that we need multiple abstractions for "a set of templates and associated resources" — it's better to make themes more flexible.

Future ideas

I've deliberately left these out of the RFC so we can focus on implementing the first step. These ideas may happen in the future, but I don't want them to prevent us from starting.

  • Some kind of registry of theme name => theme location mapping, e.g. in the config yaml
  • Theme inheritance
  • Theme clustering (so that the forum add-ons for the simple theme can be automatically added)
  • Yaml configs for themes.

Tasks

  • Theme lookup changed from single theme to list of themes
  • Themes can be looked up in four locations listed
  • API for setting theme list added
  • API for getting "default" theme list added
  • Tests
  • Documentation
  • Fix broken template references
@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 1, 2016

Member

@silverstripe/core-team does this seem like a reasonable way of addressing the need to have bootstrap-compatible-form-templates?

Member

sminnee commented Jun 1, 2016

@silverstripe/core-team does this seem like a reasonable way of addressing the need to have bootstrap-compatible-form-templates?

@kinglozzer

This comment has been minimized.

Show comment
Hide comment
@kinglozzer

kinglozzer Jun 2, 2016

Member

My understanding of this is that we’d have two “themes”, e.g:

  • framework/templates - old stuff
  • framework/themes/bootstrapforms/templates - new stuff

CMS would then enable the bootstrapforms theme, outside of CMS would just use the old templates. If you wanted to then use the bootstrap forms in your project, you could use SSViewer::set_theme(['mytheme', 'bootstrapforms']). Is that correct? It sounds like a good solution to me.

I wouldn’t let the point about “Anyone else who wanted to use bootstrap would need to repeat our work” affect the solution we choose here. Surely getting around that is still as simple as a module that someone could publish?

Member

kinglozzer commented Jun 2, 2016

My understanding of this is that we’d have two “themes”, e.g:

  • framework/templates - old stuff
  • framework/themes/bootstrapforms/templates - new stuff

CMS would then enable the bootstrapforms theme, outside of CMS would just use the old templates. If you wanted to then use the bootstrap forms in your project, you could use SSViewer::set_theme(['mytheme', 'bootstrapforms']). Is that correct? It sounds like a good solution to me.

I wouldn’t let the point about “Anyone else who wanted to use bootstrap would need to repeat our work” affect the solution we choose here. Surely getting around that is still as simple as a module that someone could publish?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 2, 2016

Member

Yeah, that's right. In the CMS we'd might use the theme system too:

SSViewer::set_theme(['admin', 'bootstrapforms'])

An then put the lion's share of the CMS templates into framework/admin/themes/admin

Member

sminnee commented Jun 2, 2016

Yeah, that's right. In the CMS we'd might use the theme system too:

SSViewer::set_theme(['admin', 'bootstrapforms'])

An then put the lion's share of the CMS templates into framework/admin/themes/admin

@chillu chillu added this to the CMS 4.0.0-alpha2 milestone Jun 13, 2016

@chillu chillu changed the title from RFC: Cascadable themes for form styling to RFC: Cascadable ("nested") themes for form styling Jun 13, 2016

@chillu

This comment has been minimized.

Show comment
Hide comment
@chillu

chillu Jun 13, 2016

Member

@kinglozzer Note that it's not "old stuff" vs "new stuff", I'd expect the current form field templates to be used by default for 4.x. Basically, we can't remove a single class from form field templates without causing major upgrade pains for anybody implementing their own website forms based on the Form API.

In order to avoid this RFC discussion getting off topic, I'm commenting about the constraints and requirements of this solution on the issue for implementing bootstrap forms over at #5677

Member

chillu commented Jun 13, 2016

@kinglozzer Note that it's not "old stuff" vs "new stuff", I'd expect the current form field templates to be used by default for 4.x. Basically, we can't remove a single class from form field templates without causing major upgrade pains for anybody implementing their own website forms based on the Form API.

In order to avoid this RFC discussion getting off topic, I'm commenting about the constraints and requirements of this solution on the issue for implementing bootstrap forms over at #5677

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 13, 2016

Member

Can we take a vote on this RFC, @silverstripe/core-team?

React with 👍 for yes, 👎 for no, :-/ (confused) for abstain.

Let's tally results Monday 20 June.

EDIT: Please vote on the original ticket, not this comment.

Member

sminnee commented Jun 13, 2016

Can we take a vote on this RFC, @silverstripe/core-team?

React with 👍 for yes, 👎 for no, :-/ (confused) for abstain.

Let's tally results Monday 20 June.

EDIT: Please vote on the original ticket, not this comment.

@stevie-mayhew

This comment has been minimized.

Show comment
Hide comment
@stevie-mayhew

stevie-mayhew Jun 13, 2016

Contributor

So the "old styles" would be marked as deprecated and then removed (possibly to their own module) in 5.0? This is simply a workaround upgrade path for 3.x -> 4.x ?

I like the idea of being able to set multiple themes and override component templates slowly so will +1 the idea of allowing multiple theme include paths, but want to clarify that these "old styles" will be removed in the future and not actively worked on?

Contributor

stevie-mayhew commented Jun 13, 2016

So the "old styles" would be marked as deprecated and then removed (possibly to their own module) in 5.0? This is simply a workaround upgrade path for 3.x -> 4.x ?

I like the idea of being able to set multiple themes and override component templates slowly so will +1 the idea of allowing multiple theme include paths, but want to clarify that these "old styles" will be removed in the future and not actively worked on?

@chillu

This comment has been minimized.

Show comment
Hide comment
@chillu

chillu Jun 13, 2016

Member

So the "old styles" would be marked as deprecated and then removed (possibly to their own module) in 5.0?

I wouldn't expect form markup (or applied classes) to change in SS4 or SS5 outside of the CMS. The bootstrap markup is quite different in terms of field holders, DOM nesting etc - a lot of forms will break if we change this, which seems unneccessary given we're discussing a way to have multiple themes (and form field templates) operate side-by-side here. We'll remove/rewrite the old CSS for CMS forms. I'm also keen to drop support for any bundled CSS files to work outside of the CMS (e.g. Form.css or DateField.css), assuming they're not used very frequently anyway, and cause us maintenance hassles. Discussion for a different RFC though.

Member

chillu commented Jun 13, 2016

So the "old styles" would be marked as deprecated and then removed (possibly to their own module) in 5.0?

I wouldn't expect form markup (or applied classes) to change in SS4 or SS5 outside of the CMS. The bootstrap markup is quite different in terms of field holders, DOM nesting etc - a lot of forms will break if we change this, which seems unneccessary given we're discussing a way to have multiple themes (and form field templates) operate side-by-side here. We'll remove/rewrite the old CSS for CMS forms. I'm also keen to drop support for any bundled CSS files to work outside of the CMS (e.g. Form.css or DateField.css), assuming they're not used very frequently anyway, and cause us maintenance hassles. Discussion for a different RFC though.

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 14, 2016

Member

I like the idea of being able to set multiple themes and override component templates slowly so will +1 the idea of allowing multiple theme include paths, but want to clarify that these "old styles" will be removed in the future and not actively worked on?

I like the idea of shifting them to a module, particularly once modules are moved to vendor/. I'm not sure if we'd necessarily deprecate the module, but those templates embed a lot of presumption about how your HTML should be, which may or may not be appropriate.

We may also want to split things up, e.g. that the templates for gridfields and their components are provided in their own theme. GridField rendering is another area with a lot of prescriptiveness.

Member

sminnee commented Jun 14, 2016

I like the idea of being able to set multiple themes and override component templates slowly so will +1 the idea of allowing multiple theme include paths, but want to clarify that these "old styles" will be removed in the future and not actively worked on?

I like the idea of shifting them to a module, particularly once modules are moved to vendor/. I'm not sure if we'd necessarily deprecate the module, but those templates embed a lot of presumption about how your HTML should be, which may or may not be appropriate.

We may also want to split things up, e.g. that the templates for gridfields and their components are provided in their own theme. GridField rendering is another area with a lot of prescriptiveness.

@stevie-mayhew

This comment has been minimized.

Show comment
Hide comment
@stevie-mayhew

stevie-mayhew Jun 14, 2016

Contributor

The point of using SemVer is that we can make breaking changes, so what we're talking about is essentially easing upgrade paths for users going from 3.x -> 4.x with a heavy reliance on the form generation for front end. Sounds like an ideal use case for multiple theme cascades which is what this RFC essentially is?

With that in mind I think the easiest use case for upgrade would be to implement this RFC, pull all of the current form styles into their own theme, and provide notes to upgraders to implement that theme (silverstripe/silverstripe-frontend-forms as a theme module includable by composer)

This would look something like the following:

Upgrading from 3.x?
If you use SilverStripe forms on the front end, you may wish to rely on the 3.x form themes which include CSS, JS and Template files. Install `silverstripe-frontend-forms@3.0.0` with composer and add the following to your configuration, assuming that you are using the `simple` theme as well.

yml:
SSViewer:
  themes:
    - simple
    - silverstripe-frontend-forms

Requirements:
  customCSS: 'silverstripe-frontend-forms/css/Forms.css'
  customJavaScript: 'silverstripe-frontend-forms/javascript/Forms.js'

Or however that will work (not sure about Requirements)

This allows an easy upgrade path, allows the breaking changes to be made without worrying about existing users and keeps our code base clean. It also allows for an upgrade path where you can then override the templates in simple which would then take precedence over the silverstripe-3-forms code.

We can then do this further in relation to GridField and other components as needed:
composer require silverstripe-frontend-gridfield@4.0.0
composer require silverstripe-frontend-forms@3.0.0

SSViewer:
  themes:
    - simple
    - silverstripe-frontend-gridfield
    - silverstripe-frontend-forms

Just making sure I've got all this correct and ensuring we're not keeping things around in the codebase (which I see causing issues further down the line) unnecessarily when alternatives exist.

Contributor

stevie-mayhew commented Jun 14, 2016

The point of using SemVer is that we can make breaking changes, so what we're talking about is essentially easing upgrade paths for users going from 3.x -> 4.x with a heavy reliance on the form generation for front end. Sounds like an ideal use case for multiple theme cascades which is what this RFC essentially is?

With that in mind I think the easiest use case for upgrade would be to implement this RFC, pull all of the current form styles into their own theme, and provide notes to upgraders to implement that theme (silverstripe/silverstripe-frontend-forms as a theme module includable by composer)

This would look something like the following:

Upgrading from 3.x?
If you use SilverStripe forms on the front end, you may wish to rely on the 3.x form themes which include CSS, JS and Template files. Install `silverstripe-frontend-forms@3.0.0` with composer and add the following to your configuration, assuming that you are using the `simple` theme as well.

yml:
SSViewer:
  themes:
    - simple
    - silverstripe-frontend-forms

Requirements:
  customCSS: 'silverstripe-frontend-forms/css/Forms.css'
  customJavaScript: 'silverstripe-frontend-forms/javascript/Forms.js'

Or however that will work (not sure about Requirements)

This allows an easy upgrade path, allows the breaking changes to be made without worrying about existing users and keeps our code base clean. It also allows for an upgrade path where you can then override the templates in simple which would then take precedence over the silverstripe-3-forms code.

We can then do this further in relation to GridField and other components as needed:
composer require silverstripe-frontend-gridfield@4.0.0
composer require silverstripe-frontend-forms@3.0.0

SSViewer:
  themes:
    - simple
    - silverstripe-frontend-gridfield
    - silverstripe-frontend-forms

Just making sure I've got all this correct and ensuring we're not keeping things around in the codebase (which I see causing issues further down the line) unnecessarily when alternatives exist.

@stevie-mayhew

This comment has been minimized.

Show comment
Hide comment
@stevie-mayhew

stevie-mayhew Jun 14, 2016

Contributor

This also would allow theme composition which is particularly exciting as it would allow you to build theme sets which rely on others, i.e. simple-widgets relying on simple which could be then overwritten using simple-widgets-awesomer from another vendor. I really like this idea and its one of the "why hasn't it happened sooner" things so excuse my brain dumps.

Contributor

stevie-mayhew commented Jun 14, 2016

This also would allow theme composition which is particularly exciting as it would allow you to build theme sets which rely on others, i.e. simple-widgets relying on simple which could be then overwritten using simple-widgets-awesomer from another vendor. I really like this idea and its one of the "why hasn't it happened sooner" things so excuse my brain dumps.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 15, 2016

Contributor

Given we want to allow modules (such as the CMS) to provide themes, is there any reason to maintain a separate "theme" package versus a module that simply provides a theme? By making themes more module-like, you can include yaml registration of new themes (among other config), which currently isn't possible with 3.x themes.

That makes redundant the whole need for a THEMES_DIR at all, since each theme can have any physical location (as registered via yaml).

E.g. imagine a module with the below structure.

mymodule:
  _config:
    - mymodule.yml
  code:
    - MyPage.php
  themes:
    theme1:
      templates:
        - Page.ss
        - Page_results.ss
    theme2:
      templates:
        - MyPage.ss

Where mymodule.yml will have:

SSViewer:
  theme_paths:
    theme1: mymodule/themes/theme1
    theme2: mymodule/themes/theme2

A site which uses the above themes could simply register such in their config.yml:

---
Name: mysite
After:
  - '#framework'
---
SSViewer:
  themes:
    - theme1
    - theme2

This provides us the ability to order and prioritise themes using normal YAML before / after prioritisation functionality.

Given that this change would be to simply allow modules to register themes, the "old" theme package doesn't need to be changed at all, and can work via automatic registration.

Note on subthemes: I guess we could use the same convention where if $LOCATION is the path to a theme, then $LOCATION_<modulename> is the location of the module subtheme, although maybe we should just have explicit "inheritable" themes rather than the current poor-mans inheritance that are subthemes. Needs some more thought...

Contributor

tractorcow commented Jun 15, 2016

Given we want to allow modules (such as the CMS) to provide themes, is there any reason to maintain a separate "theme" package versus a module that simply provides a theme? By making themes more module-like, you can include yaml registration of new themes (among other config), which currently isn't possible with 3.x themes.

That makes redundant the whole need for a THEMES_DIR at all, since each theme can have any physical location (as registered via yaml).

E.g. imagine a module with the below structure.

mymodule:
  _config:
    - mymodule.yml
  code:
    - MyPage.php
  themes:
    theme1:
      templates:
        - Page.ss
        - Page_results.ss
    theme2:
      templates:
        - MyPage.ss

Where mymodule.yml will have:

SSViewer:
  theme_paths:
    theme1: mymodule/themes/theme1
    theme2: mymodule/themes/theme2

A site which uses the above themes could simply register such in their config.yml:

---
Name: mysite
After:
  - '#framework'
---
SSViewer:
  themes:
    - theme1
    - theme2

This provides us the ability to order and prioritise themes using normal YAML before / after prioritisation functionality.

Given that this change would be to simply allow modules to register themes, the "old" theme package doesn't need to be changed at all, and can work via automatic registration.

Note on subthemes: I guess we could use the same convention where if $LOCATION is the path to a theme, then $LOCATION_<modulename> is the location of the module subtheme, although maybe we should just have explicit "inheritable" themes rather than the current poor-mans inheritance that are subthemes. Needs some more thought...

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 15, 2016

Contributor

Added my 👍 for a general go-ahead, although I feel the specifics need some finalisation. :)

Contributor

tractorcow commented Jun 15, 2016

Added my 👍 for a general go-ahead, although I feel the specifics need some finalisation. :)

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 15, 2016

Member

Given we want to allow modules (such as the CMS) to provide themes, is there any reason to maintain a separate "theme" package versus a module that simply provides a theme?

Agreed - instead we can just have a module that provides 1 theme.

Other stuff

I like relying on the config system for linking theme locations, but I think it's a bad idea to make assumptions about the root path of a module

So, instead of

 SSViewer:
   theme_paths:
     theme1: mymodule/themes/theme1
     theme2: mymodule/themes/theme2

Perhaps we do something this, relying on the packagist module name?

 SSViewer:
   theme_paths:
     theme1: "vendor/module:themes/theme1"
     theme2: "vendor/module:themes/theme2"

Then we'll be able to more easily reinterpret this when we move modules into the vendor/ path.

Member

sminnee commented Jun 15, 2016

Given we want to allow modules (such as the CMS) to provide themes, is there any reason to maintain a separate "theme" package versus a module that simply provides a theme?

Agreed - instead we can just have a module that provides 1 theme.

Other stuff

I like relying on the config system for linking theme locations, but I think it's a bad idea to make assumptions about the root path of a module

So, instead of

 SSViewer:
   theme_paths:
     theme1: mymodule/themes/theme1
     theme2: mymodule/themes/theme2

Perhaps we do something this, relying on the packagist module name?

 SSViewer:
   theme_paths:
     theme1: "vendor/module:themes/theme1"
     theme2: "vendor/module:themes/theme2"

Then we'll be able to more easily reinterpret this when we move modules into the vendor/ path.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 15, 2016

Contributor

or just

SSViewer:
  theme_paths:
    theme1: '$ModuleRoot/mymodule/themes/theme1'

We could forgo $ModuleRoot and simply ASSUME that all theme dirs are relative to module root. I guess the config shouldn't need to care about where modules exist right?

Contributor

tractorcow commented Jun 15, 2016

or just

SSViewer:
  theme_paths:
    theme1: '$ModuleRoot/mymodule/themes/theme1'

We could forgo $ModuleRoot and simply ASSUME that all theme dirs are relative to module root. I guess the config shouldn't need to care about where modules exist right?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 15, 2016

Member

If you do that, you're going to need to make the config system aware of which modules specify which config settings and pull that data in when fetching and merging config values. I think the config system is plenty complex enough.

Member

sminnee commented Jun 15, 2016

If you do that, you're going to need to make the config system aware of which modules specify which config settings and pull that data in when fetching and merging config values. I think the config system is plenty complex enough.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 15, 2016

Contributor

ModuleRoot is just vendor though right? Although I admit I haven't considered how to resolve vendor parent dir.

Contributor

tractorcow commented Jun 15, 2016

ModuleRoot is just vendor though right? Although I admit I haven't considered how to resolve vendor parent dir.

@hafriedlander

This comment has been minimized.

Show comment
Hide comment
@hafriedlander

hafriedlander Jun 15, 2016

Member

I don't see any practical difference between module/themes/theme1, vendor/module:themes/theme1 or just module:themes/theme1. But I also don't particularly see any benefit in allowing arbitrary theme dir locations.

I definitely do agree with "themes can just be modules" though - I just think hardcoding "themes are all the things that live in the theme dir" as an assumption is fine. The config system would be used for configuring other things (like any inheritance / stacking a theme does).

Member

hafriedlander commented Jun 15, 2016

I don't see any practical difference between module/themes/theme1, vendor/module:themes/theme1 or just module:themes/theme1. But I also don't particularly see any benefit in allowing arbitrary theme dir locations.

I definitely do agree with "themes can just be modules" though - I just think hardcoding "themes are all the things that live in the theme dir" as an assumption is fine. The config system would be used for configuring other things (like any inheritance / stacking a theme does).

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jun 15, 2016

Contributor

If we move to vendor-modules, we'll already need a new api for "give me the root dir of this module" anyway, so it's probably not necessary to address it here.

It's probably fine for us to say that module-themes MUST have a fixed format, if that makes the process simpler. E.g. a fixed themes dir under each module directory (whether in vendor or not).

Contributor

tractorcow commented Jun 15, 2016

If we move to vendor-modules, we'll already need a new api for "give me the root dir of this module" anyway, so it's probably not necessary to address it here.

It's probably fine for us to say that module-themes MUST have a fixed format, if that makes the process simpler. E.g. a fixed themes dir under each module directory (whether in vendor or not).

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 15, 2016

Member

@hafriedlander we still need a way of saying "theme X from module Y". So, as I understand it, your suggestion seems to be basically that we put this in the config:

vendor/module:theme1

Instead of:

vendor/module:themes/theme1

Which hardly seems like the stuff of holy wars.

Member

sminnee commented Jun 15, 2016

@hafriedlander we still need a way of saying "theme X from module Y". So, as I understand it, your suggestion seems to be basically that we put this in the config:

vendor/module:theme1

Instead of:

vendor/module:themes/theme1

Which hardly seems like the stuff of holy wars.

@hafriedlander

This comment has been minimized.

Show comment
Hide comment
@hafriedlander

hafriedlander Jun 15, 2016

Member

Why "vendor"? (And actually, I don't see why we need to say theme X from module Y - is it to handle the same theme name being in multiple modules?).

Member

hafriedlander commented Jun 15, 2016

Why "vendor"? (And actually, I don't see why we need to say theme X from module Y - is it to handle the same theme name being in multiple modules?).

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 15, 2016

Member

Why vendor?

I figure that we may as well start relying on the Composer more, and refer to modules by their Composer package names. It will be ignored for now, and very important if we move packages to the vendor/ path. If we start putting it in there now, we don't have to force a breaking change on everyone down the road.

Member

sminnee commented Jun 15, 2016

Why vendor?

I figure that we may as well start relying on the Composer more, and refer to modules by their Composer package names. It will be ignored for now, and very important if we move packages to the vendor/ path. If we start putting it in there now, we don't have to force a breaking change on everyone down the road.

@hafriedlander

This comment has been minimized.

Show comment
Hide comment
@hafriedlander

hafriedlander Jun 15, 2016

Member

Vendor isn't in composer package names. It is in the path of a "normal composer" requirement, but since we've got that : in there, that isn't a path anyway. I wouldn't expect us to ever have normal user code ever have to have "vendor" in it (any more than PHP include statements don't have vendor in them now).

Member

hafriedlander commented Jun 15, 2016

Vendor isn't in composer package names. It is in the path of a "normal composer" requirement, but since we've got that : in there, that isn't a path anyway. I wouldn't expect us to ever have normal user code ever have to have "vendor" in it (any more than PHP include statements don't have vendor in them now).

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 16, 2016

Member

How will you distinguish sminnee/template from chillu/template, then? We work with composer modules. Composer modules are only confirmed unique if specified as vendor/package.

PHP include statements make use the autoload rules. I guess we could specify an equivalent for templates, as an extra step. It seems like more work, though... That would take us closer to what Damian was advocating.

Member

sminnee commented Jun 16, 2016

How will you distinguish sminnee/template from chillu/template, then? We work with composer modules. Composer modules are only confirmed unique if specified as vendor/package.

PHP include statements make use the autoload rules. I guess we could specify an equivalent for templates, as an extra step. It seems like more work, though... That would take us closer to what Damian was advocating.

@chillu

This comment has been minimized.

Show comment
Hide comment
@chillu

chillu Jun 24, 2016

Member

I've marked the related uservoice item as "started": https://silverstripe.uservoice.com/forums/251266-new-features/suggestions/6478017-themes-should-be-more-powerful. Hamish, since you raised this on Uservoice, can you confirm that this RFC will satisfy it? It doesn't implement "codeless page types" - maybe split that out into a separate Uservoice post?

Member

chillu commented Jun 24, 2016

I've marked the related uservoice item as "started": https://silverstripe.uservoice.com/forums/251266-new-features/suggestions/6478017-themes-should-be-more-powerful. Hamish, since you raised this on Uservoice, can you confirm that this RFC will satisfy it? It doesn't implement "codeless page types" - maybe split that out into a separate Uservoice post?

@hafriedlander

This comment has been minimized.

Show comment
Hide comment
@hafriedlander

hafriedlander Jul 13, 2016

Member

PR at #5804 for initial review

Member

hafriedlander commented Jul 13, 2016

PR at #5804 for initial review

@tazzydemon

This comment has been minimized.

Show comment
Hide comment
@tazzydemon

tazzydemon Jul 18, 2016

Contributor

Personally I'm sad that bootstrap was chosen as I am a foundation supporter. Where does the switch to Bootstrap forms leave me? Its hard enough to override forms now.

I discovered that in SS3.4 my moving Forms.ss to a new Layout directory (see #5696 and silverstripe/silverstripe-userforms#483) eventually after cache rebuilds causes the CMSMain_Content.ss not to load as that too is in Includes.
This means I will have to move UserForm.ss and my template's one to Includes. I hope this gets easier. Certainly a way to switch front end forms away from Bootstrap is vitally important or Silverstripe will lose some users. Perhaps some baked in front end form element switching based on a form argument

Contributor

tazzydemon commented Jul 18, 2016

Personally I'm sad that bootstrap was chosen as I am a foundation supporter. Where does the switch to Bootstrap forms leave me? Its hard enough to override forms now.

I discovered that in SS3.4 my moving Forms.ss to a new Layout directory (see #5696 and silverstripe/silverstripe-userforms#483) eventually after cache rebuilds causes the CMSMain_Content.ss not to load as that too is in Includes.
This means I will have to move UserForm.ss and my template's one to Includes. I hope this gets easier. Certainly a way to switch front end forms away from Bootstrap is vitally important or Silverstripe will lose some users. Perhaps some baked in front end form element switching based on a form argument

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jul 18, 2016

Member

@tazzydemon if you're working on extensions to the CMS UI itself, you be best using bootstrap. However, for your own websites you'll be able to use Foundation, Bootstrap, or anything else.

SilverStripe 4 will ship with form templates for the SS3-style markup forms, and for bootstrap-style markup. Developers will be able to choose.

In addition, other developers (maybe this is a project you'd like to pick up?) will be able to make themes/modules such as "foundationforms". This can be a theme containing foundation-style templates that only cover the auto-generated forms.

People building sites will be able to select the foundationforms theme as well as another theme. In this way, the core set of form HTML can be shared across multiple foundation-based themes.

This is what we mean by a theme cascade: you can apply multiple themes, each of which will provide some of the HTML templates your site requires.

Member

sminnee commented Jul 18, 2016

@tazzydemon if you're working on extensions to the CMS UI itself, you be best using bootstrap. However, for your own websites you'll be able to use Foundation, Bootstrap, or anything else.

SilverStripe 4 will ship with form templates for the SS3-style markup forms, and for bootstrap-style markup. Developers will be able to choose.

In addition, other developers (maybe this is a project you'd like to pick up?) will be able to make themes/modules such as "foundationforms". This can be a theme containing foundation-style templates that only cover the auto-generated forms.

People building sites will be able to select the foundationforms theme as well as another theme. In this way, the core set of form HTML can be shared across multiple foundation-based themes.

This is what we mean by a theme cascade: you can apply multiple themes, each of which will provide some of the HTML templates your site requires.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jul 18, 2016

Contributor

Currently bootstrap forms will be a separate theme, and will only be turned on by default for the CMS. It won't even be enabled for front-end by default, unless you explicitly opt-in.

The default templates will still be ss 3.x style, (i.e. non-bootstrap).

In 4.0 template path resolution is going to behave a lot more explicitly than it was before; I.e. templates that exist within a certain path (e.g. Layout/Forms.ss) will only inherit / override other templates with the same path in other themes, and rendering using these templates will rely on explicitly declaring those paths. E.g. $myForm->renderWith('Layout/Forms.ss'). Userforms will of course need an update. :)

Contributor

tractorcow commented Jul 18, 2016

Currently bootstrap forms will be a separate theme, and will only be turned on by default for the CMS. It won't even be enabled for front-end by default, unless you explicitly opt-in.

The default templates will still be ss 3.x style, (i.e. non-bootstrap).

In 4.0 template path resolution is going to behave a lot more explicitly than it was before; I.e. templates that exist within a certain path (e.g. Layout/Forms.ss) will only inherit / override other templates with the same path in other themes, and rendering using these templates will rely on explicitly declaring those paths. E.g. $myForm->renderWith('Layout/Forms.ss'). Userforms will of course need an update. :)

@tazzydemon

This comment has been minimized.

Show comment
Hide comment
@tazzydemon

tazzydemon Jul 18, 2016

Contributor

Thank you both for your speedy reply. Yes this is an issue I have been having - as you well know from my pull requests, the framework one of which would have been in subtle error

I already used Ryan Wachtl’s foundation forms module and have extended it. I have mixed this with Userforms 4 and got myself into quite a pickle (as you know).

I have now found that I have to move the Userforms module’s Userform.ss into Includes and all my template’s equivalents: Userform.ss, Heroform.ss and Form.ss into Includes. This is patently wrong but matches framework, having found that ultimately the cache breaks with the CMS Main Editform (another form in Includes) when I move framework's Form.ss to Layout

I fear the form template locations are currently a tad random and look forward to some logic prevailing.

Maybe its just that I'm old :(

Contributor

tazzydemon commented Jul 18, 2016

Thank you both for your speedy reply. Yes this is an issue I have been having - as you well know from my pull requests, the framework one of which would have been in subtle error

I already used Ryan Wachtl’s foundation forms module and have extended it. I have mixed this with Userforms 4 and got myself into quite a pickle (as you know).

I have now found that I have to move the Userforms module’s Userform.ss into Includes and all my template’s equivalents: Userform.ss, Heroform.ss and Form.ss into Includes. This is patently wrong but matches framework, having found that ultimately the cache breaks with the CMS Main Editform (another form in Includes) when I move framework's Form.ss to Layout

I fear the form template locations are currently a tad random and look forward to some logic prevailing.

Maybe its just that I'm old :(

@tazzydemon

This comment has been minimized.

Show comment
Hide comment
@tazzydemon

tazzydemon Jul 18, 2016

Contributor

@tractorcow beware of the cms module form positioning if you move Form.ss to Layout. I fear they are wrong too.

Contributor

tazzydemon commented Jul 18, 2016

@tractorcow beware of the cms module form positioning if you move Form.ss to Layout. I fear they are wrong too.

@hafriedlander

This comment has been minimized.

Show comment
Hide comment
@hafriedlander

hafriedlander Jul 18, 2016

Member

@tazzydemon I tested, and there was a couple of things I needed to move. But I might have missed some, because the CMS behat tests are failing now. I'm looking into it.

Member

hafriedlander commented Jul 18, 2016

@tazzydemon I tested, and there was a couple of things I needed to move. But I might have missed some, because the CMS behat tests are failing now. I'm looking into it.

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jul 18, 2016

Contributor

Going forward, the templates will need to be in the same location, but once that location is decided, it won't matter so long as they are consistent. :P

Contributor

tractorcow commented Jul 18, 2016

Going forward, the templates will need to be in the same location, but once that location is decided, it won't matter so long as they are consistent. :P

@tazzydemon

This comment has been minimized.

Show comment
Hide comment
@tazzydemon

tazzydemon Jul 18, 2016

Contributor

I'm guessing none of this is to be retrofitted to 3.4 (which is what I am working on)?
I'm looking forward to shifting to 4 asap because it is so fast. 3.4 is definitely not.
Yes consistency. I thought just moving the framework one was the easiest and most logical but as usual it broke something else!!

Contributor

tazzydemon commented Jul 18, 2016

I'm guessing none of this is to be retrofitted to 3.4 (which is what I am working on)?
I'm looking forward to shifting to 4 asap because it is so fast. 3.4 is definitely not.
Yes consistency. I thought just moving the framework one was the easiest and most logical but as usual it broke something else!!

@tractorcow

This comment has been minimized.

Show comment
Hide comment
@tractorcow

tractorcow Jul 18, 2016

Contributor

Sorry, I'd love to back-port some logic to 3.x, but the way it's currently set up it'll be nigh impossible to move templates without regressions.

Contributor

tractorcow commented Jul 18, 2016

Sorry, I'd love to back-port some logic to 3.x, but the way it's currently set up it'll be nigh impossible to move templates without regressions.

@tazzydemon

This comment has been minimized.

Show comment
Hide comment
@tazzydemon

tazzydemon Jul 19, 2016

Contributor

As you have suggested, as long as I know and it’s consistent.

Contributor

tazzydemon commented Jul 19, 2016

As you have suggested, as long as I know and it’s consistent.

@chillu chillu referenced this issue Aug 29, 2016

Closed

Document Cascadable Themes #5924

1 of 1 task complete
@pitchandtone

This comment has been minimized.

Show comment
Hide comment
@pitchandtone

pitchandtone Jun 28, 2017

Contributor

While we've just started using this new system, there's two things that come to mind:

  1. "inherit from other themes, and define codeless page types" are probably two features not one, I suspect the latter is more interesting to devs
  2. This seems to introduce a bunch of headaches around the inheritiance idea, which from my perspective has questionable value to actual devs and only solves the issue with breaking people's current form styling.

Can someone explain why cascading themes is a good idea?

Contributor

pitchandtone commented Jun 28, 2017

While we've just started using this new system, there's two things that come to mind:

  1. "inherit from other themes, and define codeless page types" are probably two features not one, I suspect the latter is more interesting to devs
  2. This seems to introduce a bunch of headaches around the inheritiance idea, which from my perspective has questionable value to actual devs and only solves the issue with breaking people's current form styling.

Can someone explain why cascading themes is a good idea?

@pitchandtone

This comment has been minimized.

Show comment
Hide comment
@pitchandtone

pitchandtone Jun 28, 2017

Contributor

For the new form templates, can't we just be brave go to the new template and provide an additional module that could be included to override this if someone needed to stay with the old templates.

Contributor

pitchandtone commented Jun 28, 2017

For the new form templates, can't we just be brave go to the new template and provide an additional module that could be included to override this if someone needed to stay with the old templates.

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 28, 2017

Member

The primary use case of the cascaded themes so far is for adding "form-only" themes into the mix - you can define a theme that provides HTML, css, and JavaScript for say a bootstrap boilerplate, and then add site-specific templates on top of that.

The other thing is that we've always had a cascade, it's just been a stupidly hard-coded one of modules -> theme dir -> project dir.

Member

sminnee commented Jun 28, 2017

The primary use case of the cascaded themes so far is for adding "form-only" themes into the mix - you can define a theme that provides HTML, css, and JavaScript for say a bootstrap boilerplate, and then add site-specific templates on top of that.

The other thing is that we've always had a cascade, it's just been a stupidly hard-coded one of modules -> theme dir -> project dir.

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 28, 2017

Member

It would be good if you could elaborate on he problems that you're having.

Member

sminnee commented Jun 28, 2017

It would be good if you could elaborate on he problems that you're having.

@pitchandtone

This comment has been minimized.

Show comment
Hide comment
@pitchandtone

pitchandtone Jun 28, 2017

Contributor

Why would the old system not be suitable to the "form-only" issue?

Contributor

pitchandtone commented Jun 28, 2017

Why would the old system not be suitable to the "form-only" issue?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 28, 2017

Member

We're not going back to the old system on the basis of a non-specific complaint. What makes be new system so difficult for you?

Member

sminnee commented Jun 28, 2017

We're not going back to the old system on the basis of a non-specific complaint. What makes be new system so difficult for you?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 28, 2017

Member

OK, so a bit more context:

Template cascade: it's always been there, it's just less of a hack now

In SS2-3 we support templates and client resources provided by the project, by modules, by themes, and by sub-themes (e.g. "blackcandy_blog")

The "cascade" is simply the order in which these resources are searched, the first matching templating being used. In SS2-3 this order changed a few times, but it was hard-coded to:

  • Project
  • Theme
  • Sub-themes
  • Modules (in alphabetical order)

This has some issues:

  • The handling of themes, modules, and templates were quite different. There were two parallel paths for similar sets of code. It was messy.
  • The specific order that we had baked into the concrete worked for most people, but not all. It wasn't something that should be 1-setting-for-everyone, as the range of use-cases were quite diverse. Some people always used themes, some people never did.

So in response we unified the template engine's handling of modules and themes, and referenced everything as either:

  • A composer package, or
  • A subdirectory of a composer package (e.g. silverstripe/admin:/themes/cms-froms)

Internally a prioritised list of all such resources is maintained, and that defines template override rules. There's a default "ThemeManifest" that mimics the prioritisation rules of SS3, but that can be removed and you can change the order however your use-case dictates.

We had one specific issue which we decided to park until this feature was implemented, and that was the handling of bootstrap forms in the CMS. We recognised that having. Sure, we could remove the legacy forms and replace them with bootstrap forms, but what if someone wanted to use foundation, or another library that didn't work nicely with our default bootstrap forms? Being able to have a template-pack that took care of form styling for the CSS framework of your choice seemed like a nice thing to support. However it was more "the straw that broke the camel's back" than the reason we did this.

In the end we never implemented the idea of theme inheritance, instead just letting people specify and ordered list of themes.

Template namespacing

There's another change we made to template handling around this time, and that was the handling of namespaced templates. Template names generally match classnames, and with the introduction of namespaces, this meant our templates were namespaced too. In the end we decided on:

  • Use namespace in the template names too
  • Use a folder convention similar to PSR-0, where namespaces are turned into subfolders
  • Get rid of the "manifest scan" approach to finding templates in whatever directory they're found, and enforce the use of a namespace-based folder structure.

Optionally, you can use an "Includes" subfolder in this mix, but frankly I think it's a hangover from the old approach to managing template folders and should be deprecated. Others were less sure, but it's possible that this is a source of confusion.


So, we've made a bunch of changes for a bunch of reasons, and although I'm happy to admit that we might be able to improve the developer experience, we're going to do that by building on what we have an not reverting it.

Member

sminnee commented Jun 28, 2017

OK, so a bit more context:

Template cascade: it's always been there, it's just less of a hack now

In SS2-3 we support templates and client resources provided by the project, by modules, by themes, and by sub-themes (e.g. "blackcandy_blog")

The "cascade" is simply the order in which these resources are searched, the first matching templating being used. In SS2-3 this order changed a few times, but it was hard-coded to:

  • Project
  • Theme
  • Sub-themes
  • Modules (in alphabetical order)

This has some issues:

  • The handling of themes, modules, and templates were quite different. There were two parallel paths for similar sets of code. It was messy.
  • The specific order that we had baked into the concrete worked for most people, but not all. It wasn't something that should be 1-setting-for-everyone, as the range of use-cases were quite diverse. Some people always used themes, some people never did.

So in response we unified the template engine's handling of modules and themes, and referenced everything as either:

  • A composer package, or
  • A subdirectory of a composer package (e.g. silverstripe/admin:/themes/cms-froms)

Internally a prioritised list of all such resources is maintained, and that defines template override rules. There's a default "ThemeManifest" that mimics the prioritisation rules of SS3, but that can be removed and you can change the order however your use-case dictates.

We had one specific issue which we decided to park until this feature was implemented, and that was the handling of bootstrap forms in the CMS. We recognised that having. Sure, we could remove the legacy forms and replace them with bootstrap forms, but what if someone wanted to use foundation, or another library that didn't work nicely with our default bootstrap forms? Being able to have a template-pack that took care of form styling for the CSS framework of your choice seemed like a nice thing to support. However it was more "the straw that broke the camel's back" than the reason we did this.

In the end we never implemented the idea of theme inheritance, instead just letting people specify and ordered list of themes.

Template namespacing

There's another change we made to template handling around this time, and that was the handling of namespaced templates. Template names generally match classnames, and with the introduction of namespaces, this meant our templates were namespaced too. In the end we decided on:

  • Use namespace in the template names too
  • Use a folder convention similar to PSR-0, where namespaces are turned into subfolders
  • Get rid of the "manifest scan" approach to finding templates in whatever directory they're found, and enforce the use of a namespace-based folder structure.

Optionally, you can use an "Includes" subfolder in this mix, but frankly I think it's a hangover from the old approach to managing template folders and should be deprecated. Others were less sure, but it's possible that this is a source of confusion.


So, we've made a bunch of changes for a bunch of reasons, and although I'm happy to admit that we might be able to improve the developer experience, we're going to do that by building on what we have an not reverting it.

@pitchandtone

This comment has been minimized.

Show comment
Hide comment
@pitchandtone

pitchandtone Jun 29, 2017

Contributor

Firstly I'm not asking to revert anything, I just didn't see any justification for these changes and I'm trying to understand the bigger picture that you're working with. The previous comment helps this greatly, so thank you.

So to summarise:

Two issues => Two solutions:

  1. Different folder structure => Namespaced theme, no template manifest
  2. Hardcoded order => Customisable order

What I'm still not clear on is why the form issue was parked until this work was done, I don't see why the hardcoded order wouldn't have sufficed for this?

Contributor

pitchandtone commented Jun 29, 2017

Firstly I'm not asking to revert anything, I just didn't see any justification for these changes and I'm trying to understand the bigger picture that you're working with. The previous comment helps this greatly, so thank you.

So to summarise:

Two issues => Two solutions:

  1. Different folder structure => Namespaced theme, no template manifest
  2. Hardcoded order => Customisable order

What I'm still not clear on is why the form issue was parked until this work was done, I don't see why the hardcoded order wouldn't have sufficed for this?

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jun 29, 2017

Member

In retrospect I guess we could have done form styles as a module. It would have been harder to get precise about the ordering of those templates as vs any in framework. And it wouldn't have been possible to enable/disable them for different UIs. So if we had separate form styles for CMS from the front-end site, we'd have had to have bundled them with an admin theme.

The other thing would have been that "an admin theme" would have been more awkward - it would have had to have been it's own composer package checked out to the themes folder.

None of these problems are insurmountable they just added up to a messier picture.

Member

sminnee commented Jun 29, 2017

In retrospect I guess we could have done form styles as a module. It would have been harder to get precise about the ordering of those templates as vs any in framework. And it wouldn't have been possible to enable/disable them for different UIs. So if we had separate form styles for CMS from the front-end site, we'd have had to have bundled them with an admin theme.

The other thing would have been that "an admin theme" would have been more awkward - it would have had to have been it's own composer package checked out to the themes folder.

None of these problems are insurmountable they just added up to a messier picture.

@pitchandtone

This comment has been minimized.

Show comment
Hide comment
@pitchandtone

pitchandtone Jul 12, 2017

Contributor

Thanks for the synopsis @sminnee makes more sense now. I'll raise specific issues if we find them.

Contributor

pitchandtone commented Jul 12, 2017

Thanks for the synopsis @sminnee makes more sense now. I'll raise specific issues if we find them.

@sminnee

This comment has been minimized.

Show comment
Hide comment
@sminnee

sminnee Jul 12, 2017

Member

No worries — there's a lot of new stuff SS4 that's appeared over the last couple of years! ;-)

Member

sminnee commented Jul 12, 2017

No worries — there's a lot of new stuff SS4 that's appeared over the last couple of years! ;-)

@patricknelson

This comment has been minimized.

Show comment
Hide comment
@patricknelson

patricknelson Oct 18, 2017

Contributor

Deleted my comments regarding twig/blade blocks due to not being very relevant here, sorry about that. Posted here here instead: #7498

Contributor

patricknelson commented Oct 18, 2017

Deleted my comments regarding twig/blade blocks due to not being very relevant here, sorry about that. Posted here here instead: #7498

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