Add option for extra powered by logo in footer #3153
Conversation
✅ Deploy Preview for ons-design-system-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rmccar
changed the title
The focus state is broken on the ONS logo |
Co-authored-by: rmccar <[email protected]>
Co-authored-by: rmccar <[email protected]>
Co-authored-by: rmccar <[email protected]>
|
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 
|
Co-authored-by: rmccar <[email protected]>
Co-authored-by: rmccar <[email protected]>
|
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
force-pushed
the
feature/3134-add-option-for-additional-logo-in-footer
branch
from
f4444fc to
292575c
Compare
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. |
src/components/footer/_footer.scss
Outdated
| } | ||
| &__extra-poweredby svg, | ||
| img { | ||
| height: 30px !important; |
There was a problem hiding this comment.
Where did we get the size from? is the important really needed here?
There was a problem hiding this comment.
Dina approved the size
There was a problem hiding this comment.
This would cause an issue where the ons logo would be bigger when there is 2 logos and smaller when there is just the ons logo so it wouldn't be consistent in size between pages
There was a problem hiding this comment.
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!
Focus state is now an issue, it includes the margin you have added when there are two logos |
rmccar
changed the title
I also have the same sentiment :) |
|
|
||
| | Name | Type | Required | Description | | ||
| | --------- | -------------- | -------- | --------------------------------------------------------------------------------------- | | ||
| | logoImage | HTML or string | false | Any HTML to render an image for example embedded `<svg>` or `<img>`. | |
There was a problem hiding this comment.
"or string" can be removed from this, it doesn't accept a string. Also the description could do with updating the word "Any" can be removed
| <div class="ons-footer__poweredby ons-u-mb-m{% if extraPoweredByLogo %} ons-footer__extra-poweredby ons-grid--flex{% endif %}"> | ||
| <a class="ons-footer__poweredBy-link{% if extraPoweredByLogo %} ons-u-mr-l{% endif %}" {% if lang == "cy" %} href="https://cy.ons.gov.uk/"{% else %} href="https://www.ons.gov.uk/"{% endif %}> | ||
| {{ onsLogo | safe }} | ||
| </a> | ||
| {{ extraPoweredByLogo | safe }} | ||
| </div> |
There was a problem hiding this comment.
| <div class="ons-footer__poweredby ons-u-mb-m{% if extraPoweredByLogo %} ons-footer__extra-poweredby ons-grid--flex{% endif %}"> | |
| <a class="ons-footer__poweredBy-link{% if extraPoweredByLogo %} ons-u-mr-l{% endif %}" {% if lang == "cy" %} href="https://cy.ons.gov.uk/"{% else %} href="https://www.ons.gov.uk/"{% endif %}> | |
| {{ onsLogo | safe }} | |
| </a> | |
| {{ extraPoweredByLogo | safe }} | |
| </div> | |
| <div class="ons-footer__poweredby ons-u-mb-m{% if extraPoweredByLogo %} ons-footer__extra-poweredby ons-grid--flex{% endif %}"> | |
| <a class="ons-footer__poweredBy-link{% if extraPoweredByLogo %} ons-u-mr-l{% endif %}" {% if lang == "cy" %} href="https://cy.ons.gov.uk/"{% else %} href="https://www.ons.gov.uk/"{% endif %}> | |
| {{ onsLogo | safe }} | |
| </a> | |
| {{ extraPoweredByLogo | safe }} | |
| </div> |
There was a problem hiding this comment.
We discussed this ticket and the other related ticket in the tech session, to improve standardisation, the implementation and naming of the logos in the footer should mimic the new implementation in the header - as we would plan to do this eventually anyway, it makes more sense to go with that implementation now rather than have an intermediary change, only to break it again a bit later.
|
This pull request is going to be closed as the original request has now been updated and a new PR will be opened. |
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.