nhsuk-frontend icon indicating copy to clipboard operation
nhsuk-frontend copied to clipboard

Feature/breadcrumbs update

Open pflynny opened this issue 2 years ago • 8 comments

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

pflynny avatar Sep 07 '21 07:09 pflynny

Does this Breadcrumb change require a change to the padding-top on the <main class="nhsuk-main-wrapper" id="maincontent"> element? See next comment...

chrimesdev avatar Sep 10 '21 08:09 chrimesdev

Spacing from the Breadcrumb and <h1> on this page:

Screenshot 2021-09-10 at 09 33 20

Looks a lot more detached than on this page:

Screenshot 2021-09-10 at 09 34 04

Due to nhsuk-u-padding-top-0 being added to the <main class="nhsuk-main-wrapper" id="maincontent"> element.

chrimesdev avatar Sep 10 '21 08:09 chrimesdev

@karlgoldstraw are you OK to look at this change from a design point of view?

chrimesdev avatar Sep 10 '21 08:09 chrimesdev

Hey @pflynny any update on this?

chrimesdev avatar Oct 21 '21 13:10 chrimesdev

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 avatar Oct 21 '21 14:10 pflynny

@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?

chrimesdev avatar Oct 22 '21 10:10 chrimesdev

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?

pflynny avatar Nov 03 '21 09:11 pflynny

I've tested and added the sibling option to the code if you'd like to see it working.

pflynny avatar Nov 03 '21 09:11 pflynny

Closing this pull request, once V7 has shipped we can come back to the breadcrumbs.

pflynny avatar Dec 16 '22 17:12 pflynny