Require the use of a class to apply any govuk elements styles #134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| // Fieldset is used to group more than one .form-group | ||
| fieldset { | ||
| // .fieldset is used to apply styling to fieldsets and to group more than one .form-group | ||
| .fieldset { |
There was a problem hiding this comment.
Are we happy with changes like these? I can see people now writing <div class="fieldset"> which then breaks accessibility for a bunch of people. Given the overall simplicity of our pages I’m not worried about selector complexity impacting performance so would it be better to have fieldset.fieldset as the selector? Anything that enforces accessibility from within our styling is a good idea (imo of course).
There was a problem hiding this comment.
I'm not sure that is likely to happen, but I am happy to require that this class is only applied to fieldsets. At the moment it's just a full width clearing wrapper, once the fluid widths have been removed from form controls, the width:100% can be removed here too.
Don’t set the base font-size to 19px for the main content area. A later commit will add parent classes to wrap common elements, to ensure the size for body copy is 19px. The govuk_template sets the base font size to be 16px, update the em function in helpers to use 16px its context. Where the em function is used and the second argument is 16, remove it as this is now the default. Where the em function is used and the context was previously 19px, adjust the sizing accordingly.
Also update example to show this.
Create a new .table class selector, use the existing .numeric class for right aligning table cells and headers. Update data snippets to use these new classnames.
Instead create new classnames; .details, .details-summary, .details-summary-text and .details-summary-arrow. Reduce nesting in the details partial. Update the details summary example page, progressive disclosure example and the polyfill to use these new classnames.
Add a fieldset class to apply styling to field sets. Nest .form-date input styles under the .form-date parent class. Remove form ‘helpers’ section as fieldset now uses a class, legends still require work and the textarea styles are applied when .form-control is used. Further work needs to be done to add a classname and fix legends, add TODOs where appropriate.
Bump up the font size to 19px on .text, otherwise this needs redefining all over the place. This will require a wrapper with the class of .text to be used to set a 19px default for any elements within .text. Move the max-width setting to paragraphs within these .text blocks. This will also enable anyone using markdown, or not wishing to add class attributes to each element in a text block to use the same pattern, for example, for headings and lists. The elements page stylesheet also sets a max-width for lists, to avoid having to use grid layout to constrain the text on its pages. This will also allow the prototype kit to have an additional stylesheet which can extend common element styles, using the .text scope.
…ment This encourages the use of the fieldset element, which improves form accessibility.
gemmaleigh
force-pushed
the
dont-style-element-selectors
branch
from
83a34e8 to
1ea2f8a
Compare
|
Closing this PR in favour of renaming |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Create new classnames for
.hr,.fieldset,.details,.details-summary.Require a parent class of
.textfor scoped typography styles.These changes mean that the govuk_frontend_toolkit can be imported (as main.scss does) followed by govuk_elements and it will have no effect on the project importing it, until one of its classes are used.
Rather than apply a 19px base to
<main>, this has been moved to .text, so if you'd like to use a 19px base, then the.textwrapper must wrap text blocks.Assuming govuk_template isn't being used (its element selectors are duplicated in #131), then the only remaining places where element selectors are used are:
elements/reset.scss. This was copied from alphagov/static and should be removed (but this is out of the scope of this PR).cc. @robinwhittleton, @dsingleton - it would be good to get these changes merged first, as they affect most of the template files, then I can submit additional pull requests for the remaining changes in #133.