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

fix Illustration and IllustrationBlock alt color on dark mode #5981

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

oxcened
Copy link
Contributor

@oxcened oxcened commented Apr 30, 2023

Motivation

When dark mode is turned on, the alt text rendered by the Illustration and IllustrationBlock components is not visible.
On dark mode, the text color inherited is text-primary-dark, which is not visible on the white background.

Change

Applying a fixed text-primary ensures that the text is always visible regardless of the current theme.

Demonstration

Source of the images have been removed for the sake of testing.

Current behaviour:

Screenshot 2023-04-30 at 17 29 58

Screenshot 2023-04-30 at 17 32 03

Fixed behaviour:

Screenshot 2023-04-30 at 17 30 02

Screenshot 2023-04-30 at 17 32 06

@github-actions
Copy link

github-actions bot commented Apr 30, 2023

Size changes

📦 Next.js Bundle Analysis for react-dev

This analysis was generated by the Next.js Bundle Analysis action. 🤖

Three Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/404 77.33 KB (🟡 +7 B) 181.08 KB
/500 77.33 KB (🟡 +7 B) 181.07 KB
/[[...markdownPath]] 78.92 KB (🟡 +7 B) 182.67 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 10% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@lunaleaps
Copy link
Contributor

Is it possible to also make the alt text dark mode when the theme is dark mode?

@lunaleaps lunaleaps self-assigned this Sep 26, 2023
@oxcened
Copy link
Contributor Author

oxcened commented Sep 27, 2023

Is it possible to also make the alt text dark mode when the theme is dark mode?

Not sure I understood your question. The alt text does currently inherit the dark mode, but since the background is fixed to white, it is not readable when dark mode is on.

@lunaleaps
Copy link
Contributor

@oxcened Oh I guess I was asking if the alt text background could also turn dark during dark mode, that way we don't need to address the text color.

@oxcened
Copy link
Contributor Author

oxcened commented Sep 28, 2023

@oxcened Oh I guess I was asking if the alt text background could also turn dark during dark mode, that way we don't need to address the text color.

Ah I got you. We could do that, but as far as I can tell, all illustration images have a white background, and I'm not sure it would look very clean.

Screenshot 2023-09-28 at 09 31 56 Screenshot 2023-09-28 at 09 32 02

Otherwise you could keep the white background on the image container, and set the dark background on the image only.

Screenshot 2023-09-28 at 09 40 44

Not sure there any other solutions?

@harish-sethuraman
Copy link
Collaborator

There are two illustrations

  • single illustration that has a white bg in image so text isnt visible in dark mode
  • illustration block that has a parent div with rounded white bg

in the single illustration bg white is not needed at all? it doesn't show up until the image is not loaded. so ideally the text is visible when bg is removed. (can remove bg-white?)

in the block we can consider having text-primary so that the text is visible in both modes and we do not interfere with the background white of the image that has rounded borders? (can add text-primary to image?)

@oxcened
Copy link
Contributor Author

oxcened commented Sep 28, 2023

in the single illustration bg white is not needed at all? it doesn't show up until the image is not loaded. so ideally the text is visible when bg is removed. (can remove bg-white?)

Yeah it is different because the background is applied directly to the img. So as long as the images have their own white background, the style has no visible effect. I think this is currently true for all images, so it depends whether it might change in the future I guess?

in the block we can consider having text-primary so that the text is visible in both modes and we do not interfere with the background white of the image that has rounded borders? (can add text-primary to image?)

I think this is the best option. Currently the PR applies it to the outer div instead of the img, though.

@oxcened
Copy link
Contributor Author

oxcened commented Nov 6, 2023

hey @lunaleaps @harish-sethuraman I did some changes according to your comments.

  • Since single illustration does not need a white background, I went ahead and removed it, together with the text-primary class since the background now goes along with the theme.
  • While in Illustration block I moved the text-primary class to the img itself.

What do you think?

@lunaleaps
Copy link
Contributor

Thanks for fixing, this looks good to me!

@lunaleaps lunaleaps merged commit 8727204 into reactjs:main Nov 6, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants