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

feat(root): add dark variants of Accessibility, Styles images #1359

Merged
merged 2 commits into from
Feb 20, 2025

Conversation

GCHQ-Developer-299
Copy link
Contributor

@GCHQ-Developer-299 GCHQ-Developer-299 commented Feb 11, 2025

(this is based on the work of #1326)

Summary of the changes

Add dark variants of accessibility, styles pages PNGs. Add helper function passImage to set image based on theme.

Added index pages to each images/ folder to make the importing of images tidier.

This PR does not include images for:

  • Each Component page
  • Patterns pages (pending image confirmation from design team)

Related issue

issue #1348

Checklist

Copy link

@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch 4 times, most recently from a946455 to b4395bb Compare February 14, 2025 10:08
@GCHQ-Developer-299 GCHQ-Developer-299 changed the title feat(root): add dark variants of Component Gallery images feat(root): add dark variants of Component Gallery, Accessibility, Styles images Feb 14, 2025
@GCHQ-Developer-299 GCHQ-Developer-299 marked this pull request as ready for review February 14, 2025 10:18
@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch from b4395bb to b84b236 Compare February 14, 2025 10:22
@GCHQ-Developer-741
Copy link
Contributor

Component Gallery looks fine, but I'm seeing a few issues on the other pages:

  1. Some images are only displaying the dark mode variant. Seeing this on /styles/layout-spacing, /styles/templates, /styles/focus-indicator, /accessibility/needs/invisible, /accessibility/needs/motor, /accessibility/needs/neurodiversity, /accessibility/needs/visual, /accessibility/needs/general.
    image

  2. Some images aren't displaying when switching to dark mode. Only seeing this at /styles/layout-spacing.
    image

  3. Some images haven't been created correctly. Only seeing this at /accessibility/needs/motor.
    image

@GCHQ-Developer-741
Copy link
Contributor

Don't know if this is part of the ticket or was discussed, but are the .mp4 files going to be changed to dark mode as well?

Copy link
Contributor

@GCHQ-Developer-847 GCHQ-Developer-847 left a comment

Choose a reason for hiding this comment

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

Spotted a few more things to add to @GCHQ-Developer-741’s points. I think these will mostly require Figma changes though

  1. On the /accessibility/needs/neurodiversity page, the images on the left side of the page look quite fuzzy (especially compared to the ones on the right).

  2. On the /accessibility/needs/visual page, when viewed in dark mode, the image on the left has red label text on the first text field (doesn’t match the light mode image or the developed ic-text-field).

  3. On the /styles/layout-spacing page, in the dark mode image with the subtitle “Group components into containers…”, the grid and container are not really visible so it’s difficult to understand what the image is showing.

  4. On the /styles/focus-indicator page, the white part of the focus indicator in the image is not displayed. Also the image in the “Links” section is not showing at all (both light and dark mode).

Also, the text in a lot of the dark mode images in the component gallery use a different font to when they are in light mode. However, it might take quite a bit of time to fix and it’s not really that noticeable, so I think it would be fine to leave them for now and sort it at a later date?

And I think it would be nice to have the image names in camelCase if possible? It would make them easier to read so it would be quicker to find certain images if we need to make changes in future. However I don't think it's essential and would be happy for them to stay as they are if making them camelCase isn't very easy :)

@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch 5 times, most recently from 002e29b to f0279b6 Compare February 17, 2025 14:24
@GCHQ-Developer-299
Copy link
Contributor Author

@GCHQ-Developer-847 @GCHQ-Developer-741 thanks for your comments. I've had a go at addressing them today (updating some of the poorly exported images, switching to camelCase) and here are the outstanding issues from my perspective:

  • Where dark mode aren't as expected, these will need figma changes so I would have to go back to Design. Would this prevent this PR going in for pre-release? One upside of merging this in would be that the Design team would be able to more easily see which images work and which don't, and update them for us.
  • In cases where the dark mode image is displaying in Light Mode, i'm not sure what's happening here. The image files are being correctly imported and passed into the image array. I want to say it's a quirk of the github deployment but I haven't debugged through it to see if it's a code failure

I might put ComponentGallery in its own, smaller PR since it's all good to go, so that something gets into this release

@GCHQ-Developer-741
Copy link
Contributor

GCHQ-Developer-741 commented Feb 17, 2025

@GCHQ-Developer-847 @GCHQ-Developer-741 thanks for your comments. I've had a go at addressing them today (updating some of the poorly exported images, switching to camelCase) and here are the outstanding issues from my perspective:

  • Where dark mode aren't as expected, these will need figma changes so I would have to go back to Design. Would this prevent this PR going in for pre-release? One upside of merging this in would be that the Design team would be able to more easily see which images work and which don't, and update them for us.
  • In cases where the dark mode image is displaying in Light Mode, i'm not sure what's happening here. The image files are being correctly imported and passed into the image array. I want to say it's a quirk of the github deployment but I haven't debugged through it to see if it's a code failure

I might put ComponentGallery in its own, smaller PR since it's all good to go, so that something gets into this release

I would agree that it could maybe go in for pre-release and the adjustments could be made after the fact on the Figma side, but the dark mode images displaying in Light Mode shouldn't. If it is a deployment issue (I noticed the layout example I included is working now after the changes you made) then hopefully should be fine, but I don't think it should be on the site if they aren't switching correctly.

Also, one thing I have noticed is that the third image I added earlier (on the motor needs page) doesn't line up when put next to each other. Just recording for future amendment as it seems to be a Figma change

@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch 2 times, most recently from 9c54bd7 to bc40094 Compare February 18, 2025 14:32
@GCHQ-Developer-299 GCHQ-Developer-299 changed the title feat(root): add dark variants of Component Gallery, Accessibility, Styles images feat(root): add dark variants of Accessibility, Styles images Feb 18, 2025
@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch from bc40094 to 98710c4 Compare February 19, 2025 10:04
Copy link
Contributor

Choose a reason for hiding this comment

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

One for designers but this .png is smaller than its light variant, and I'm not sure on the box shadow used on the images as it's not something we implement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the image, although it now has a subtle background to all the text

@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch from 98710c4 to 2f4d814 Compare February 20, 2025 11:57
gd2910
gd2910 previously approved these changes Feb 20, 2025
Add dark versions of the accessibility page figure images, and an index.tsx to export them
Update DoDontCaution to take array of images and switch them based on theme

1348
Add index file and dark variants of images in Styles pages

1348
@GCHQ-Developer-299 GCHQ-Developer-299 force-pushed the 1348-transfer-dark-mode-images branch from 2f4d814 to 6054a98 Compare February 20, 2025 14:49
@gd2910 gd2910 merged commit 0a38ece into v3.0.0/develop Feb 20, 2025
3 checks passed
@gd2910 gd2910 deleted the 1348-transfer-dark-mode-images branch February 20, 2025 15:21
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