search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

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

Closed sminnee closed 8 years ago

sminnee commented 8 years ago

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:

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.

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:

Ideas I don't like:

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.

sminnee commented 8 years ago

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

kinglozzer commented 8 years ago

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

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 commented 8 years ago

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 commented 8 years ago

@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 https://github.com/silverstripe/silverstripe-framework/issues/5677

sminnee commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

sminnee commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

hafriedlander commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.

sminnee commented 8 years ago

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).

OOOOH, I think you were misunderstanding me. I was meaning $vendor/$package, e.g. silverstripe/framework:bootstrapforms. Not the actual string "vendor".

hafriedlander commented 8 years ago

Yeah - just picked that. That makes more sense. I still don't really understand why we need to map theme keys to theme names at all, except for the limited case of "I have multiple modules with the same theme name inside them and need to disambiguate", but assuming everyone else disagrees, yes "$vendor/$package:$themename" makes sense.

sminnee commented 8 years ago

Except for the limited case of "I have multiple modules with the same theme name inside them and need to disambiguate"

It seems like the same argument as class namespaces. Why wouldn't you want to namespace templates as well?

I don't necessarily think that we need an intermediary mapping—my preference would be to just refer to themes by their longer names—but I feel that if we're going to start creating larger numbers of smaller themes, the risk of a name collision will go up, and designing a system without namespacing would be a failure to learn from our past mistakes. ;)

hafriedlander commented 8 years ago

Because we don't have multiple places the can have different use statements - in the end, they're all imported into the SSViewer scope. You need the vendor and the module in the key. Which may have been what you were saying?

hafriedlander commented 8 years ago 
SSViewer:
  theme_paths:
    hafriedlander/foo: 'hafriedlander/hamishsmodule/themes/foo'

But then how is that advantageous over just passing "hafriedlander/hamishsmodule/themes/foo" straight to SSViewer?

SSViewer::set_theme(['hafriedlander/hamishsmodule/hamishstheme', ....])

It's a little bit shorter, and I guess lets you override if needed?

sminnee commented 8 years ago

OK – I agree with you. I want to see namespaces. I don't particularly see the need for a mapping config. I also like the colon separating the package name and the subfolder name, as it makes it clear that it's not as simple as just a folder path.

hafriedlander commented 8 years ago

That's good, because I was coming around to agreeing with you :). I still think "templates as a special directory" can go away.

We probably also want to be able to do template inheritance, since there's a sort-of built in cascade in 3.x (module/templates overridden by $selectedtheme/templates), and I could see that using yaml, but probably can go in a seperate story.

sminnee commented 8 years ago

We probably also want to be able to do template inheritance

In the MVP you can do this by putting the parent template in your set_theme call(). Agree that some kind of config for themes would make sense.

hafriedlander commented 8 years ago

It also means you won't be able to have multiple modules all "pitch in" their own templates to a common theme.

So you couldn't have

silverstripe-blog/themes/simple/template/BlogPost.ss silverstripe-forum/themes/simple/template/FormPost.ss silverstripe-simple/themes/simple/templates/Page.ss

(Where silverstripe-X is vendor == silverstripe and module == X)

With the config, you could:

silverstripe-blog/_config/theme.yml
SSViewer:
  theme_paths:
    silverstripe/simple: 'silverstripe/blog/themes/simple'

silverstripe-simple/_config/theme.yml
SSViewer:
  theme_paths:
    silverstripe/simple: 'silverstripe/simple/themes/simple'

Although right now you'd have to pick whether that was a conflict (two modules providing the same theme), or a merge, and there wouldn't be any clear priority if exactly the same template existed in both paths on merge.

sminnee commented 8 years ago

I would assume that you'd do that like this:

SSViewer::set_theme(['silverstripe/blog:simple', 'silverstripe/forum:simple', 'silverstripe/simple']);

Yes, it puts the onus on the project developer to put them together. But I didn't think that a showstopper. If anything, magic that auto-assembles stuff is likely to be more confusing.

I also don't think that this is used very often.

hafriedlander commented 8 years ago

Ah yeah, that'd work fine.

You could also use configuration, but have a list of paths:

silverstripe-blog/_config/theme.yml
SSViewer:
  themes:
    silverstripe/simple:
      - 'silverstripe/blog/themes/simple'

silverstripe-simple/_config/theme.yml
Before: *
-----
SSViewer:
  themes:
    silverstripe/simple:
      - silverstripe/simple/themes/simple'

That would let you just do SSViewer::set_theme('silverstripe/simple') - but managing list merges in various config fragments can get a bit fraught.

sminnee commented 8 years ago

My preference would be to wait until some demonstrated developer need rather than pre-emptively introducing complexity.

That said, trying to figure out how to reference themes had ended up with 5 different signatures (see updated original post), so it's not a panacea.

sminnee commented 8 years ago

@silverstripe/core-team okay everyone I've taken a stab at amending the proposal to incorporate the discussion above. I've left a number of ideas stated as "for later", and clarified the syntax that you would use to specify themes.

Can we field any objections to this so that we can start building?

tractorcow commented 8 years ago
/:/: Reference a theme contained within the subfolder of a package. Eg: "silverstripe/framework:/themes/bootstrapforms"

If we simply say "you must use the specific folder structure" then we can skip this, right? i.e. <vendor>/<module>/themes/<theme>

sminnee commented 8 years ago

If we simply say "you must use the specific folder structure" then we can skip this, right?

One of my goals was to avoid having "theme templates" and "normal templates" - instead, we'd have a single system for grabbing themes from various locations. Hrm, I haven't really been very explicit in stating that, though.

For modules, this would be achieved by specifying silverstripe/forum, say, as a theme. We may want to have some kind of a shortcut that automatically adds any included module to the end of the cascade—this would retain the current behaviour.

So, what I didn't have a good way of saying, is "what about the templates in mysite?" For most other modules, you can just say the package name, but mysite isn't typically a separate composer package, and so you'd probably just refer to this as /mysite.

In fairness, silverstripe/framework:/themes/bootstrapforms isn't as important; I kind of just included this syntax for completeness.

sminnee commented 8 years ago

OK I spoke with @hafriedlander and @tractorcow and we agreed a few final points that have made their way into the RFC. See the "Default themes" and "Theme bundles" sections above.

It's fair to say that those on @silverstripe/core-team who have previously voted didn't know what they now have voted for, but hopefully it's not too far out of line with the previous discussion. I'd like to be able to start work on this next week, so it would be great to field any serious objections over the weekend if there are any.

hafriedlander commented 8 years ago

Yup. 👍 from me now. There's lots of extra stuff I want to add (like the inheritance) but that can go in future stories once we see how this gets used in practise.

tractorcow commented 8 years ago

👍 happy with the addition of "default themes" concept. It means that adding modules to existing projects will be easier going forward.

chillu commented 8 years ago

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 commented 8 years ago

PR at https://github.com/silverstripe/silverstripe-framework/pull/5804 for initial review

tazzydemon commented 8 years ago

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 https://github.com/silverstripe/silverstripe-userforms/issues/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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

hafriedlander commented 8 years ago

@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 commented 8 years ago

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