Skip to content
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

Add basic styling for typographic elements #382

Closed
wants to merge 4 commits into from
Closed

Conversation

selfthinker
Copy link
Contributor

@selfthinker selfthinker commented Jan 9, 2017

This adds styling to standard typographic elements without the need to use classes directly on the elements.
Please discuss.

What problem does the pull request solve?

I find having all HTML elements styled in a sensible way really important. I call that "basic styles".
It is important because it makes it easier for non-frontend developers to use the correct markup. It makes the general look and feel more consistent. It reduced the amount of markup and CSS necessary. And it saves time by making simple markup look good by default.

It is similar to what the govuk-govspeak class from GOV.UK Components provides, although that had a different intention.

When I set up my first project with Elements I overcame this restriction in a way which some people didn't like. This PR would make that unnecessary (unless we're using slimmer/static and can use govuk-govspeak).

What does it do?

This adds styling to standard typographic elements (i.e. headings, lists, links, code, strong) without the need to use classes directly on the elements.
This is done scoped under the govuk-basic-styles govuk-prose class which can be added to the body or main or just a single component. That makes it backwards-compatible and people can opt in to use them.

Inconsistently, some basic (but non-scoped) styles already exist for:

  • tables
  • paragraphs (spacing)
  • details/summary
  • links (although links are styled via govuk_template not via Elements)

After this change, it would stay inconsistent that one type of styles are scoped and others are not. To change that would be a breaking change whether you chose all of them to be opt in or opt out.

This is not a full PR as it is meant to start a discussion. What would be necessary to complete this PR:

  • testing
  • documentation

Later other missing elements could also be styled (e.g. definition lists or quotes) which you would expect to be styled.

How has this been tested?

This has not been tested yet as this PR was created to have a discussion about the basic idea first. I will test properly and write documentation if people are happy with the basic idea.

I have tested all the relevant elements inside a div with the new class in the styleguide that comes with Elements. I have only tested in one browser as I don't believe there will be any browser-related issues. Please let me know if I should test anything more.

What type of change is it?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Put an x in all the boxes that apply

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

@dankmitchell
Copy link

I like the intention of this PR. For me, coming onboard very recently it's very hard to get your head around all the different gems/packages that include varies components and styles so I'd personally prefer to see a gov.uk normalise file(like normalise.css or reset.css) that includes all these basic rules, since they are gov.uk styles then there shouldn't be a case where someone wouldn't need to use it. I agree that's it's cumbersome to add 'heading-large' to all H1's when we have a strict rules on how H1's are styled anyway.

@dsingleton
Copy link
Contributor

👍 to the overal idea

As you mentioned it's pretty similar to the GOV.UK Publishing govspeakcomponent, and preceding .govspeak scope. Both have been used in the past to style non-govspeak content, effectively treating them as a scope like this, so there's a strong need.

Small nitpick on the naming - is there anything more descriptive than govuk-basic-styles? The implementation now (and how govspeak has been used) feels like 'text', 'body-copy' or 'document' -styles. 'basic' feels more ambiguous about what it applies, and something you layer on top of, rather than a scope you opt in to. Like I said, nitpick, but worth some thought on the name when introducing something. Also, you mentioned forms - but that feels a bit beyond what this scope should be doing, and we're hoping to move form UI into components in the future.

One of the things isolated components depend on (including GOV.UK Publishing components, and future GOV.UK Frontend Components) is that element level styles aren't widely applied, or they risk overriding internal styles of a component.

How do you see this working when applied to a main scope, say if you wanted to use components interleaved with basic-style'd content?. I'm thinking of a page like this, https://www.gov.uk/register-to-vote, where there's a button component in the middle of text content. Would the suggested use be like this?:

<div class="govuk-basic-styles">...</div>
<%= component.button('Start Now', startnow=true) %>
<div class="govuk-basic-styles">...</div>

For a body scope I can imagine situations in admins/previews/tools where this'd be useful, but wouldn't want anyone using it on service pages, as it would break the header/footer. I think the doc would need to emphasise inline/component use case, rather than on main or body (but still allow them).

Appreciate it's a long reply with a fair bit of detail, happy to chat about this in person if it's more useful :)

@mcgoooo
Copy link

mcgoooo commented Jan 10, 2017

gonna ask the question, in an ideal world, should the style not just be on the h1, h2 etc? bootstrap style.

The styles on our website are pretty simple, and in a normal run of things, i don't think we should need to write any css (or use classes for css) i have no idea how we would get to that though without essentially just having an major version number revision

@edwardhorsford
Copy link
Contributor

edwardhorsford commented Jan 10, 2017

This feels reasonably sensible, and I'd likely use it. However, I'm wary of going down a path of having two ways of doing everything. I feel like we'd get to a point where some pages in a service would use this, and some would use the classes, purely depending on when the page was built. Do we want that?

Other comments:

  1. I have similar usage to this already in the prototype kit. In that case I had some markdown to style, so couldn't use class names. You can see what I came up with here.

We're considering extending markdown support to the rest of the kit, so a .govuk-basic-styles or similar would be useful.

  1. "we have a strict rules on how H1's are styled anyway." - this isn't necessarily true. We have strict rules around body text and hints, etc but heading sizes depend on the page and content. For example, I'd expect a start page to have 48px h1, but within a service I'd likely use 36px. So we'd want to consider carefully what defaults this applies.

Edit: also curious why this doesn't style body or paragraph with @core-19 as a sensible default.

@dsingleton
Copy link
Contributor

dsingleton commented Jan 10, 2017

gonna ask the question, in an ideal world, should the style not just be on the h1, h2 etc? bootstrap style.

We want to be as close as possible, while balancing maintainability and the ability to iterate frontend code. I think having a 'texty' (or 'prose' as @gemmaleigh suggested) scope, that can be tightly applied, gives us the ability to write 'normal' HTML.

The difficulty to applying element styles globally is that it makes it very difficult to build on top of them. If all h1s are styled one way by default, it makes it much more difficult to create variants (eg, an h1 for a manual) in a way that isn't totally dependant on the implementation of the global style. Thats something we've been actively trying to get away from on GOV.UK, and from research with services would make upgrading more difficult for them.


This PR would be a good topic for the next FE meeting, lets side some time aside and we can discussion the topics raised here. I think it'd be more productive in person than back and forth over GH.

@tvararu
Copy link
Contributor

tvararu commented Jan 10, 2017

One of the things isolated components depend on (including GOV.UK Publishing components, and future GOV.UK Frontend Components) is that element level styles aren't widely applied, or they risk overriding internal styles of a component.

^ @dsingleton took some of my words out of my mouth here.

However, I'm wary of going down a path of having two ways of doing everything.

@edwardhorsford I don't think you can completely avoid that, because the web platfom is a tool that is widely used for different usecases. IMO chiefly among them are Documents and Applications, which have different needs.

To over simplify what I think:

  • Applications love reusable components.
  • Documents love implicit base styling.

The difficulty to applying element styles globally is that it makes it very difficult to build on top of them.

@dsingleton I think the reason this is difficult on the web platform is because of how easy it makes it to almost irreversibly override the primitives, and there's no easy way to explicitly request the basic ones afterwards.

I'm not a native developer, but I assume that if you tried to monkey patch the default behaviour of UITextView and every other base class in the iOS toolkit you'd get some confused eyebrows, in most applications. Unless, you were creating a Bootstrap type abstraction layer that makes it easier to create your specific flavour of applications.

While I initially knee-jerkingly had my nose up at this (as a primarily Applications-concerned web developer), I can see how it would be useful and save a lot of time for most of our use cases, which are Documents.

@selfthinker
Copy link
Contributor Author

@dsingleton, I'm fine with changing the name, I wasn't too sure about that myself. Although I would be fine with calling it something related to copy/document I would find it a bit odd because we're only giving back the styles we have deliberately removed (i.e. the default browser styles).

On the one hand I think there should be a default styling for form elements as well, but on the other hand I understand that it's not quite the same. Basic styling of e.g. inputs and labels brings the problem with it that you would need to override those in case of checkboxes and radio buttons.
I have already done the changes but deliberately didn't put it into this PR because it's easier to discuss a smaller and more straightforward change.
Do you know of a case where an input field or a button doesn't look like a standard GOV.UK element? Maybe I just don't know the code base well enough to know those cases but I haven't come across one yet.

I mentioned putting the scope on various elements as examples. Although putting it on the body is a possibility, you most likely wouldn't want to do that when using govuk_template. (And you would quickly realise that.) Putting it on main is probably a more realistic scenario, especially for typical apps for documents. In services you could either not apply it at all or only on select sections.

I can see multiple solutions to the isolated components vs global styles issue:

  1. Don't use the basic-styles class if you're relying heavily on components.
  2. Either reset everything per component. That's probably a bit drastic but it's a possibility.
  3. Or reset only what you know you need to reset because those basic styles will be known to you. The downside is that basic styles might change and you would need to re-check this and potentially adjust from time to time.

I personally don't like the idea that components set all the possible styles. There are some basic things they should inherit, e.g. the font, font size and text colour. That way it will be much less code and easier to keep consistent. They should only override those if their styling diverges from that.

I have only ever seen issues with basic styles in rare cases in the past if they include something fancy which is hard to override. I've heard multiple times from other GDS developers that it was causing issues (@dsingleton said "it makes it very difficult to build on top of them"). I'd be interested to know, do you have any real life examples when it was causing problems?

In an ideal world I would also prefer to not use two different ways of styling typographic elements. I would prefer to only style h1 directly etc. But there are two reasons why we shouldn't do that in this case:

  1. We would break a lot of things as such a change wouldn't be backwards compatible.
  2. I know that the removal of all basic styling is quite important to a lot of people. Adding them back only when scoped and giving teams a choice will be the best compromise.

Although @cfq pointed out to me that it will be more difficult to override the basic styles because they are scoped.
E.g. if you wanted to make an h1 look like an h2, you could not simply add a .heading-large class as .basic-styles h1 has a higher specificity. It would be easier to override if it wasn't scoped.

@edwardhorsford, I came up with a similar solution as linked in the description above: https://github.com/alphagov/content-performance-manager/blob/master/app/assets/stylesheets/_base.scss

also curious why this doesn't style body or paragraph with @core-19 as a sensible default.

Paragraphs are already styled accordingly. The @core-19 is on the main element and everything inside inherits it unless it's overwritten. Paragraphs have basic spacing.

@tvararu, your apps vs documents comparison makes sense to me. Maybe I'm only such a strong believer in basic styles because I've never built a proper modern web app (not counting DokuWiki as its main purpose is to provide documents and its "appy" part is not very extensive).

@dsingleton
Copy link
Contributor

To try and summarise where this is at:

I think everyone agrees some kind of basic-styles scope is useful, and there's solid prior art inside and outside GDS. I think the questions raised are:

  • With a basic styles scope there's a question on how many elements it should cover?

  • How would a scope interact with a component system, can they be embedded within a scope?

  • How does applying a basic styles scope to body/html* affect building isolated components?

(* at this point I think it's actually strongly opinionated reset/baseline set of styles, rather than a scope)

@selfthinker
Copy link
Contributor Author

I'm happy to keep the discussion about a body/html scope out of this PR. Although during our frontend meeting it was clear that some people would like to explore that, at least to the extend of experimenting with it. @dsingleton suggested that someone could provide a demo with which we could experiment. I'd be happy to provide that, although it's likely to happen only in 2 or 3 weeks. (I'm in a new team and should concentrate on work within the team at least in the beginning.)

What was also discussed (later after the meeting and in Slack) was to have at least some rudimentary, less opinionated basic styles which should at least let components inherit very basic things for easier consistency, like font, font size and colour.
But again, this is out of scope of this particular PR -- but probably interesting to know for people following this.

I have removed the to do list item "basic styles for form elements and buttons" from the description. I think it makes more sense to potentially discuss that separately in a separate PR.

@dsingleton
Copy link
Contributor

👍 I'm happy with this as a scope 'texty' content. I think it would make sense to update the name to like text or prose and make it more explicit. But otherwise

If folks want to apply it at that level they still can, but we should make clear we're not supporting that, and the potential risks. @gemmaleigh: where's a good place in elements docs for that?

I think the outstanding steps here are agreeing a name, and documenting the usage. I think thats it, and then this is mergable?


@dsingleton suggested that someone could provide a demo with which we could experiment.

I think some code demos would help focus the discussion :) I'd be interested in seeing how:

  • Using the scope on body affects the header and footer of govuk_template
  • Existing GOV.UK Elements components (like forms or phase banners) would work within a scope like this.

@selfthinker
Copy link
Contributor Author

I guess finding a good name is the most difficult bit of this PR. ;-)
It should definitely start with govuk-, options following that could be:

  • typography
  • prose
  • copy
  • document
  • text
  • styled
  • ...?

Regarding the documentation, I think it either belongs to the top or the bottom of the Elements typography page. I'm leaning towards the top.

I will update this within the next few days.

@edwardhorsford
Copy link
Contributor

In the interest of readability, something like .govuk-styled-text or .govuk-text-styles?

@selfthinker
Copy link
Contributor Author

selfthinker commented Feb 1, 2017

In the frontend meeting the majority just decided to go with govuk-prose.
I will update the PR soon accordingly.

@selfthinker
Copy link
Contributor Author

selfthinker commented Mar 3, 2017

Sorry for the long wait. I have finally found some time to finish this.
I have renamed the class to govuk-prose and added some documentation and examples.

A couple of things I was unsure of when I wrote the documentation:

  • Is the bottom the right place or should it be at the top?
  • I'm not very happy with the heading "Use styling automatically". Can anyone think of something better?
  • Should we warn people about adding the class to their body? Or is it enough not to mention it and having a simple example with a div?
  • Is it good to list which elements are affected? I'm only wary of the potential maintenance and that it could be forgotten to be updated if other elements would get added to it.

@selfthinker selfthinker changed the title [Discuss] Add basic styling for typographic elements Add basic styling for typographic elements Mar 3, 2017
@@ -214,6 +215,23 @@ <h3 class="heading-medium" id="typography-hidden-text">Hidden text (progressive
</div>


<h3 class="heading-medium" id="typography-class">Use styling automatically</h3>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about - Use scoped typographic styles

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the word 'scoped' in there is good. But I'm not sure if the whole expression makes it clear to people what it means, especially what the consequences and benefits are.

Thinking from a different angle, maybe this would fit?
"How to not use classes on every element"

<h3 class="heading-medium" id="typography-class">Use styling automatically</h3>
<div class="text">
<p>You can use the <code class="code">govuk-prose</code> class around a body of prose. Then certain elements inside will be styled automatically without the need to add specific classes to each of them.</p>
<p>That will apply to most typographic elements: headings, lists, links (without govuk_template), code and strong. Tables, paragraphs, links (with govuk_template) and horizontal rules are already always styled without that class.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is clear.

The new scope, .govuk-prose will apply styles to headings, lists, links and code and strong elements.

We should include examples of each of these in the snippet.

GOV.UK template sets global link styles, in addition to this GOV.UK elements sets global styles for tables, paragraphs and horizontal rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that especially the mentioning of govuk_template is unclear. I'm not sure how important the distinction is because I don't know how many services are using Elements without govuk_template.

What about the following?

That will apply to most typographic elements: headings, lists, links, code and strong. Tables, paragraphs and horizontal rules are already always styled without that class (and if you are using govuk_template, that includes links).

I have used examples of all of those in the snippet, except

  • most headings -- I thought just one type of heading (h3 in this case) is enough and adding more might be distracting
  • links -- the app which displays the example snippet includes govuk_template, so there won't be any difference for links if they are within the govuk-prose class or not

@selfthinker
Copy link
Contributor Author

I have just had another stab at this. A few weeks ago I met with Jen Lambourne, a technical writer here at GDS, to discuss improvements to the documentation for this PR. Unfortunately I forgot some of the things she said and the notes she sent me were not complete.

She said she'd prefer it if that section was at the top:

I would put close to the top so that users can see how to handle the correct thing automatically without manually working through the doc and then finding out they could have automated it at the end.

If you're unhappy with that, I'd suggest moving the section to the bottom again but leaving a paragraph where the section is now and linking it to the bottom section.

About warning people to not put the class on the body or not:

Always warn/repeat if it's going to cause issues. No harm in warning them and having a simple example.

I tried to rephrase the text about which elements are affected. I hope it's clearer now, but am not sure it's clear enough.

selfthinker and others added 4 commits June 15, 2017 09:03
This adds styling to standard typographic elements
without the need to use classes directly on the elements.
This is done scoped under the `govuk-prose` class
which can be added to the `body` or `main` or just a single component.
That makes it backwards-compatible and people can opt in to use them.
Also add examples of h1 and h2 styles within this scope.
This combines the styling from headings, body copy, links and lists.
@gemmaleigh
Copy link
Contributor

I've renamed this section to 'Scoped typographic styles'.

I'd like it to sit underneath lists - as i'd prefer it if examples of each of these were shown first, before an example of using a the .govuk-prose class - combining the examples above - headings, lists, links.

I'm still wary of this as it'll mean these styles are harder to override, as @cfq mentioned.

Here's a screenshot of the updated example:

typography gov uk elements

Copy link

@dankmitchell dankmitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement IMO

@gemmaleigh gemmaleigh modified the milestones: 3.1.0, 3.1.1 Jun 16, 2017
@selfthinker
Copy link
Contributor Author

We discussed this in the GDS frontend meeting on 21 June. Here is the part of the meeting minutes provided by @maxf:

Anika: Nick had some objections on pending PR. One concern is we might be a situation where we might have to support people not understanding and unexpected things can happen. Anika: we should make it clear. The risk is lower than the need. I also know that people dont read. Writing it there might not work.

Main problem is about using components standalone vs in prose. This might leadto very different rendering of the component.

Not sure if there's a programmatic way to warn users. JavaScript warnings? No. We could end up with those in live services. Maybe console.log().

Paul: we have some components are often in prose. We're struggling to separate when conponents are on their own or if theyre in the flow of the document (eg you want to show if something in important).

Oli: white list of prose scope and what components should be used where?

Paul: similar to what happens when you include one component inside another.

[discussion on how to manage styling of components inside other components, or inside prose]

outcome:

  • request to add discussion points on Anika's PR
  • suggestion that we could put other component code in the prose code as a proof of concept.

@selfthinker
Copy link
Contributor Author

I have decided to give up on this. There seems to be too much resistance.
If anyone else wants to keep on fighting for this and wants to provide arguments and tests (like mentioned above "put other component code in the prose code as a proof of concept"), please feel free to adopt this PR.

@gemmaleigh gemmaleigh removed this from the 3.1.1 milestone Jul 5, 2017
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-elements-review-pr-382 September 25, 2017 10:57 Inactive
@dsingleton
Copy link
Contributor

Hi folks 👋

I unsubscribed from notifications for this PR 😢 - but it's still showing up in my https://github.com/pulls/mentioned

It looks like this PR isn't actively owned, a tag out of date, and is conflicting with master. Any chance the PR could be closed? Leaving the branch for someone to pick up in the future? 😄

@NickColley
Copy link
Contributor

NickColley commented Jun 20, 2018

Hey David! :)

We're investigating a similar feature in GOV.UK Frontend with https://github.com/alphagov/govuk-frontend/blob/master/docs/installation/installing-with-npm.md#global-styles and we'll be investigating this use case more.

So I'll close this out for now, thanks everyone.

@NickColley NickColley closed this Jun 20, 2018
@dsingleton
Copy link
Contributor

💖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants