Open NickColley opened 6 years ago
we wen't through this discussion when we standardised component api, previous version of button macro had boolean (https://github.com/alphagov/govuk-frontend/commit/4559379446391bfcfb8ef620ff987c0b183ba63d#diff-c8d55831d340874284fc322fd388fca6R14) That was clashing with other components where boolean wouldn't work
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
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.
@igloosi can you explain what you mean? The commit you linked to doesn't make it clear why that does not work.
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.
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...
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.
Two more use cases raised #1150 and #1151.
Just to add why this would be important for us -
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.
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.
@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:
I think these fixtures are used for the exported 'specs' (https://github.com/alphagov/govuk-frontend/issues/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 https://github.com/alphagov/govuk-frontend/issues/460#issuecomment-475599158
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?
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
We could do:
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