nhsuk-frontend
nhsuk-frontend copied to clipboard
Feature/breadcrumbs update
Description
Add the updated breadcrumb that has been beta-tested on the live site, we've seen it working live on the new hub pages, so now it's time to get the update rolled out throughout the rest of the site.
Context for the update #6
Checklist
- [x] Tested against our testing policy (Resolution, Browser & Accessibility)
- [x] Follows our coding standards and style guide
- [ ] CHANGELOG entry
Does this Breadcrumb change require a change to the padding-top on the <main class="nhsuk-main-wrapper" id="maincontent">
element? See next comment...
Spacing from the Breadcrumb and <h1>
on this page:
Looks a lot more detached than on this page:
Due to nhsuk-u-padding-top-0
being added to the <main class="nhsuk-main-wrapper" id="maincontent">
element.
@karlgoldstraw are you OK to look at this change from a design point of view?
Hey @pflynny any update on this?
I was unsure what to do regarding the page spacing issue, it will require an update to <main class="nhsuk-main-wrapper" id="maincontent">
– this will be a breaking change though as any page that doesn't have breadcrumbs will require a padding-top
applied. What do you think, is best to modify <main class="nhsuk-main-wrapper" id="maincontent">
?
@pflynny I don't think changing the spacing on the main
container the best approach, as you said not every page will use Breadcrumbs but every page will use main
(it could also be a breaking change). I think we should be looking at the spacing on the Breadcrumb component, I wonder if @bencullimore has any thoughts?
As we don't want to alter the main
container, the other options I think we could look at are using the sibling selector or a negative margin on the breadcrumbs. Both have their considerations, using the sibling nav is clean, but does have a dependency on HTML elements being structured as nhsuk-breadcrumb + nhsuk-width-container > nhsuk-main-wrapper
. The NHS has a well-defined page structure, so this should be reliable. The other option is negative margin, but as this is on the main
container it will cause a repaint and reflow of all the child elements, causing a slower render time – although with the speed of browsers this might not be an issue. What do you think?
I've tested and added the sibling option to the code if you'd like to see it working.
Closing this pull request, once V7 has shipped we can come back to the breadcrumbs.