-
Notifications
You must be signed in to change notification settings - Fork 335
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
Better define relationship between headings, error message and hints #684
Conversation
src/fieldset/fieldset.yaml
Outdated
@@ -7,4 +7,5 @@ examples: | |||
data: | |||
legend: | |||
text: What is your address? | |||
classes: govuk-fieldset__legend--xl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking (since can be changed later) but this exposes internals (#460)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's consistent with the way we do it everywhere else at the minute.
|
||
// Modifiers that make labels look more like their equivalent headings | ||
|
||
.govuk-label--xl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we used the core heading classes would we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer that it's separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention behind this is to decouple the idea of what's a heading with the visual hierachy on the page - reframing the problem so that heading-xl
is 'a heading which is extra large', and label--xl
is 'a label that is extra large'.
You can therefore have an medium label which is not a heading, and you could also (theoretically) have a label with no additional styling that is a heading. Using the core heading classes muddies the semantics again.
There are some additional benefits in that we don't actually want any of the margins that the core heading classes give us, so at the minute we have to override them - whereas this allows us to define them separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think it's important to think about how the user views these, both label--xl
and heading--xl
is the same thing visually. heading--xl
shouldnt concern itself about semantics, but only how it presents as a heading.
I'm mostly concerned about:
- how we keep the two in sync
- impact on file size
- another interface to learn
That said, if it's not easy to use the core classes, this seems a reasonable approach.
Other benefit is that decoupling them makes them easier to delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But my comment was pretty much entirely about how we want the user to think about them – if they're thinking of legends and labels as headings when they are not headings then that is not helpful – understanding how to mark them up becomes a lot simpler if you can keep the distinction clear.
Visually, it's true that they're the same font size and weight, but the margins are completely different. We did go down the route of adding a mapping of xl
, l
to their respective font sizes, etc but abandoned it as it seemed premature at the time.
I'd like to avoid blocking this PR if possible given how long it's taken to get this work done, but adding a mapping or some sort of abstraction is definitely something we can revisit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have prefixed my comment with that it wasnt blocking.
Just wanted to understand that using the core classes was not possible, since I think this is very complex to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's totally possible, but I don't think it's actually desirable in this case, for the reasons outlined above.
changelog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy with this :)
We are aiming to decouple the markup of the label as the page heading from the styling, by removing any styling from the heading and delegating all styling responsibilities to the label.
a742466
to
c13793a
Compare
@igloosi happy with c13793a? |
👍 |
This decouples the markup of the label as the page heading from the styling, by removing any styling from the heading and delegating all styling responsibilities to the label.
This:
govuk-label--xl
,govuk-fieldset__legend--xl
) that make the legend or label the same size and weight as the equivalent headinglegend.isPageHeading
for fieldsets, which defines whether the legend text should be wrapped in anh1
–<legend><h1>Have you changed your name?</h1></legend>
isPageHeading
for the label component, which defines whether the label itself should be wrapped in anh1
–<h1><label>What is your National Insurance number?</label></h1>
h1
s in both contexts which effectively 'reset' the h1 such that it has no effect – i.e. theisPageHeading
flag should have no effect on the styling of the question.You can see all combinations of here.