Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unsafe inline in CSP policy and update svg fill #1349

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Apr 8, 2024

What is the context of this PR?

This removes unsafe-inline from CSP policy, removes inline CSS "fill" from our SVG components and adds svg native fill property to these. One unit test had to be amended. Some of our SVGs (Logos) use inline styles for colour fills, however these do not work without explicitly allowing the unsafe-inline CSP policy which is considered unsafe. We needed to get the SVGs updated to not use CSS.

How to review

Visually check SVG logos in different "test_theme_" schemas, check if svg components are looking the same after the change.

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@petechd petechd marked this pull request as ready for review April 8, 2024 09:28
@VirajP1002
Copy link
Contributor

I noticed one of the logos looked slightly different. Does something need to be changed? Not sure if it's on my end.

Main:
image

Your branch:
image

@petechd
Copy link
Contributor Author

petechd commented Apr 10, 2024

I noticed one of the logos looked slightly different. Does something need to be changed? Not sure if it's on my end.

Main: image

Your branch: image

Well spotted, there were some remaining CSS fill properties in that file, removed now, thanks.

Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SVGs are displayed great for me 👍

@VirajP1002
Copy link
Contributor

All but one looks good now! Just one alignment issue on test_theme_ukhsa_ons:

Main:
Screenshot 2024-04-10 at 11 39 51

Your branch:
Screenshot 2024-04-10 at 11 40 05

Not too sure if it's just for me

@liamtoozer
Copy link
Contributor

Quick follow-up question from this for @VirajP1002 & @rmccar - were you testing this locally? I tested locally but the images seemed to be aligned for that schema. Were you testing in a staging/test env instead? I just wanted to double-check what I should be looking for next time 👀

@rmccar
Copy link
Contributor

rmccar commented Apr 11, 2024

Quick follow-up question from this for @VirajP1002 & @rmccar - were you testing this locally? I tested locally but the images seemed to be aligned for that schema. Were you testing in a staging/test env instead? I just wanted to double-check what I should be looking for next time 👀

I was just testing locally, try checking out the commit before the last one and launching the test_theme_ukhsa_ons schema. Thats all I did, maybe you missed it? Then check out the latest one and they should be aligned

@liamtoozer
Copy link
Contributor

Quick follow-up question from this for @VirajP1002 & @rmccar - were you testing this locally? I tested locally but the images seemed to be aligned for that schema. Were you testing in a staging/test env instead? I just wanted to double-check what I should be looking for next time 👀

I was just testing locally, try checking out the commit before the last one and launching the test_theme_ukhsa_ons schema. Thats all I did, maybe you missed it? Then check out the latest one and they should be aligned

Ah, ignore me, I just double-checked in staging - I didn't realise that the bottom SVG always needs to look indented from the top SVG. I think I saw them both aligned locally and thought that was the expected layout. Thanks for your help 👍

@rmccar
Copy link
Contributor

rmccar commented Apr 11, 2024

Quick follow-up question from this for @VirajP1002 & @rmccar - were you testing this locally? I tested locally but the images seemed to be aligned for that schema. Were you testing in a staging/test env instead? I just wanted to double-check what I should be looking for next time 👀

I was just testing locally, try checking out the commit before the last one and launching the test_theme_ukhsa_ons schema. Thats all I did, maybe you missed it? Then check out the latest one and they should be aligned

Ah, ignore me, I just double-checked in staging - I didn't realise that the bottom SVG always needs to look indented from the top SVG. I think I saw them both aligned locally and thought that was the expected layout. Thanks for your help 👍

Nah its wrong in main too, they're both supposed to be next to each other but yeah sounds like you're seeing the right thing. If you check this branch out now you should see them how they're supposed to be

@liamtoozer
Copy link
Contributor

liamtoozer commented Apr 11, 2024

Nah its wrong in main too, they're both supposed to be next to each other but yeah sounds like you're seeing the right thing. If you check this branch out now you should see them how they're supposed to be

Yep looks all good now, thanks 👍

@petechd
Copy link
Contributor Author

petechd commented Apr 11, 2024

I've also tested this in my staging and looking good so will merge it now.

@petechd petechd merged commit a2e758e into main Apr 11, 2024
16 checks passed
@petechd petechd deleted the create-svgs-without-inline-css branch April 11, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants