ONSdigital / design-system

ONS Design System
https://service-manual.ons.gov.uk/design-system
MIT License
28 stars 17 forks source link

Add option for extra powered by logo in footer #3153

Closed precious-onyenaucheya-ons closed 1 month ago

precious-onyenaucheya-ons commented 2 months ago

What is the context of this PR?

Fixes #3134 Changes the footer component to allow users to supply an extra logo to the powered by section. Some styling were added to ensure these logos stay on the same line and behave appropriately on smaller screens.

How to review this PR

Spin up the website and head to the 'footer' component, the example-footer-with-extra-poweredby-logo on the page is the one to look at.

run yarn test --testNamePattern="macro: footer" & yarn test-visual

Checklist

This needs to be completed by the person raising the PR.

netlify[bot] commented 2 months ago

Deploy Preview for ons-design-system-preview ready!

Name Link
Latest commit acdbff58f84c25a1fe1b08e7444975ec9e9f51d0
Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/6659cd88964d9d0008d018d0
Deploy Preview https://deploy-preview-3153--ons-design-system-preview.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rmccar commented 2 months ago
Screenshot 2024-04-24 at 09 13 25

The focus state is broken on the ONS logo

rmccar commented 2 months ago

Looking at the SVG it has some space on the left so on small screens its not aligned with the ONS one. This looked ok before because you had the line in there. If you want to remove the line I dont think it should be cropped out, the code for it should be removed from the svg altogether but keeping the line might be easier and as I'm guessing its part of the official logo should probably be kept

Screenshot 2024-04-26 at 11 26 32
rmccar commented 2 months ago

The focus state is still an issue, I think to solve it the best way would be to add the ons-icon--logo class back to the SVG and remove this from the footer css

  &__poweredBy-link,
  &__additional-poweredBy-link {
    &:focus {
      .ons-icon--logo {
        @extend %a-focus;
      }
    }
  }
rmccar commented 2 months ago

We also have an issue with an empty link being rendered just before the powered by link div. The ons-footer__poweredby-logo class on that div should be removed too because it no longer is just one logo. Might need a bit of refactoring in the macro

Screenshot 2024-04-29 at 11 27 15

rmccar commented 2 months ago

The filename for the new example will now need to be updated

balibirchlee commented 2 months ago

Not sure if this has changed so ignore if it has, but originally when I spoke to Dina about this ticket she said that the the second logo should sit beneath the first one, not next to it

alessioventuriniAND commented 2 months ago

Not sure if this has changed so ignore if it has, but originally when I spoke to Dina about this ticket she said that the the second logo should sit beneath the first one, not next to it

Can you get either Joe or Dina to review this? They need to confirm if the Logos are ok to be displayed as they are. Happy to approve once you confirm their feedback :)

precious-onyenaucheya-ons commented 1 month ago

Not sure if this has changed so ignore if it has, but originally when I spoke to Dina about this ticket she said that the the second logo should sit beneath the first one, not next to it

I had a chat with Dina and she prefers to keep the logo next to the first one. However, She was concerned about potential differences in logo heights, but I handled that by resizing the logos in the CSS file.

rmccar commented 1 month ago
Screenshot 2024-05-09 at 09 47 46

Focus state is now an issue, it includes the margin you have added when there are two logos

adi-unni commented 1 month ago

Looks good to me, I prefer this layout to the one originally suggested.

Just to say (so no action required from your side) I'm gonna add a note to our tech session card about variable names, it's not in the scope of this ticket and it's not to do with your code at all so no concerns there!! I just don't really like the 'poweredBy' name, I feel like it's a bit unclear especially in comparison to the header logo section which is functionally very similar. Maybe that's just me though!

I also have the same sentiment :)

alessioventuriniAND commented 1 month ago

This pull request is going to be closed as the original request has now been updated and a new PR will be opened.