Macros Component API should not require internal classes for modifiers

[NickColley]

Macros abstract our class names, if we were to change our class names, macro usage would break for our users.

In our button examples, we pass the internal button modifier class to make a button into a start button.

We could consider passing a boolean instead so that if we choose to change how our classes work, macros still function.

For example instead of

- name: start
  data:
    text: Start now button
    classes: 'govuk-c-button--start'

We could do:

- name: start
  data:
    text: Start now button
    start: true

See also the GOV.UK Publishing component: http://govuk-static.herokuapp.com/component-guide/button/start_now_button

Link to code: https://github.com/alphagov/static/blob/a9a040a72d475ef7015c1c9e690a5d77687fcfd8/app/views/govuk_component/button.raw.html.erb#L1

We constructed the class based on booleans, then finally merge with classes and inject into the markup.

Edit:
I put together an example showing how you can restructure the macro to avoid the confusing forking logic.

This approach only forks on individual parts of the template, which is easier to reason about.

https://glitch.com/edit/#!/flag-based-macro-approach?path=views/index.html:15:26

[kr8n3r]

we wen't through this discussion when we standardised component api, previous version of button macro had boolean (4559379#diff-c8d55831d340874284fc322fd388fca6R14)
That was clashing with other components where boolean wouldn't work

[joelanman]

yeh at the time we thought MVP, classes will work. However @NickColley makes a good point, that this is fragile - if we ever change our class names, it would break.

we also discussed something like

variant: 'start'

which is more flexible

[NickColley]

Interesting idea, we could consider modifier: 'start' if we wanted to align with BEM.

I think the reason I prefer Boolean flags is that you may want to do other things than just add a modifier class.

[NickColley]

@igloosi can you explain what you mean? The commit you linked to doesn't make it clear why that does not work.

[36degrees]

A good example of where the convention 'breaks' is the way the list component used to work - there are multiple boolean possibilities but only one of them is possible at a time and the order of precedence is undefined.

I think we discussed moving to a 'modifier' approach but it's not always clear how that would work if you wanted to legitimately 'combine' modifiers - e.g. perhaps a label that is both --bold and --large and so we went with the simplest option which was to use the classes functionality we needed to provide anyway.

[richardTowers]

I think a modifiers list of strings could provide all of the benefits of classes without the drawback of having to specify fragile classnames.

Start button example:

{{ govukButton({text: "Start now button", modifiers: "start"}) }}

Large, bold label (fictional) example:

{{ govukLabel({text: "Some label", modifiers: "large bold"}) }}

I don't know how powerful nunjucks' macro system is mind you...

[36degrees]

Closing this for now – let's see if this is something that comes up in user research or once we've got some experience rolling out breaking changes.

[aliuk2012]

Two more use cases raised #1150 and #1151.

[penx]

Just to add why this would be important for us -

• we want to make govuk-react utilise govuk-frontend where possible
• the most likely route that would work for us would be to use govuk-frontend as CSS modules via govuk-frontend-react
• when using CSS modules, the actual CSS class names of a component are not known to the parent component, as they are hashed (or similar)
• as such, all class names for a component/block should only be used from within the nunjucks template for that component/block. Any modifiers classes should be accessed by passing in property to the template rather than listing class names

[timpaul]

This is an interesting proposal, but we'd need to decide which of the many classes that it's possible to legitimately apply to a component should be turned into parameters. And of course, a new parameter, no matter how simple, is a new thing for users to discover and remember.

[vanitabarrett]

We’ve decided to close this card as we think we’ve done the work to fix this issue in specific components now (for example: the button example in the issue description - the button component now has an isStartButton macro option).

It’s possible there are other components which would benefit from a tidy-up too - we should create separate tickets for these as and when we notice them/they get raised on support.

[penx]

@vanitabarrett

re #1150 and #1151 (which were closed in favour of this), I'm not sure if this has been considered fixed for date input errors and widths, but the fixtures are certainly still using class names:

govuk-frontend/src/govuk/components/date-input/date-input.yaml

Lines 103 to 120 in e0d5aad

	- name: with errors only
	data:
	id: dob-errors
	fieldset:
	legend:
	text: What is your date of birth?
	errorMessage:
	text: Error message goes here
	items:
	-
	name: day
	classes: govuk-input--width-2 govuk-input--error
	-
	name: month
	classes: govuk-input--width-2 govuk-input--error
	-
	name: year
	classes: govuk-input--width-4 govuk-input--error

I think these fixtures are used for the exported 'specs' (#1830), so I really think the fixtures should be using the isStartButton type modifiers rather than class names, in order to aid the creation of ports that don't follow these class names, and for reasons I detailed above in #460 (comment)

[owenatgov]

Re-opening this as upon revisiting, we don't feel we have an adequate strategy for component modifiers.

@penx has outlined how this impacts the input class above. This came up again when adding the inverse button variant.

We'd like to revisit this work in a future release to come up with a consistent approach to how we handle component modifiers, avoiding boolean API attributes or applying modifiers via class where possible. We have some thoughts on this, presently mostly in line with @richardTowers's string based modifier list above. @querkmachine has also suggested splitting modifier classes based on category eg: a theme attribute for colour, a size attribute for specific size etc.

We'd also like to investigate modifier priority for clashing modifiers eg: what happens if both the inverse and secondary modifier classes are applied to the button component?