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

Add color issues section #146

Merged
merged 37 commits into from
Jan 9, 2024
Merged

Add color issues section #146

merged 37 commits into from
Jan 9, 2024

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Nov 30, 2023

Description

Adds a section with space for more info on known color issues. Currently, only the Gamut mapping issue is documented- the Alpha caveats should be added with the related PRs.

Fixes #143

Steps to test/reproduce

  1. Open the page on a wide screen, see that the section defaults to open.
  2. Open the page on a narrow screen, see that the section defaults to closed.
  3. Open/close is not responsive, so changing the viewport won't change the state
  4. When the contrast doesn't meet AA Normal, it uses the same low contrast colors as the ratios.

Show me

image image

Copy link

netlify bot commented Nov 30, 2023

Deploy Preview for oddcontrast ready!

Name Link
🔨 Latest commit fee5ca9
🔍 Latest deploy log https://app.netlify.com/sites/oddcontrast/deploys/659d64b8a8e9f000089ca755
😎 Deploy Preview https://deploy-preview-146--oddcontrast.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.

@jamesnw jamesnw requested a review from jgerigmeyer November 30, 2023 18:59
@jamesnw
Copy link
Contributor Author

jamesnw commented Nov 30, 2023

This will need some design help from @stacyk or @dvdherron - there was some conflicts with the other definition list in use. I added the styles to the component, but assume they should be located elsewhere.

@jamesnw
Copy link
Contributor Author

jamesnw commented Dec 13, 2023

Note to the designers- I added a second issue in #145, which adds some additional complexity, so it may be worth styling with that content in mind.

jamesnw and others added 11 commits December 13, 2023 15:44
* main:
  sveltekit does not support vite v5 yet
  pin back stylelint for now
  Automated dependency upgrades
  Automated dependency upgrades
  Bump @adobe/css-tools from 4.3.1 to 4.3.2
* color-issues:
  sveltekit does not support vite v5 yet
  pin back stylelint for now
  Automated dependency upgrades
  Automated dependency upgrades
  Bump @adobe/css-tools from 4.3.1 to 4.3.2
src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
@stacyk
Copy link
Member

stacyk commented Dec 21, 2023

@jgerigmeyer I am assigning this to you because I see you left comments but haven't approved this PR yet.

@stacyk stacyk assigned jgerigmeyer and unassigned stacyk and SondraE Dec 21, 2023
Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

I thought this was an issue that was fixed, but I see that Safari adds their own marker, so what I thought was just a thing in progress was actually a browser difference.
Screenshot 2023-12-21 at 3 19 25 PM

I'll see what we can do since I use Safari as a primary, might be easier for me to test.

Comment on lines 35 to 40
transform: rotate(0deg);
transition: all 1s ease-out 100ms;

[open] & {
transform: rotate(90deg);
}
Copy link
Member

Choose a reason for hiding this comment

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

@mirisuzanne @dvdherron Is there a bug in Safari that doesn't allow this before pseudo element to transition when we add the [open] attribute? I am able to see the transitions if I just toggle the properties (see screen recording) but not when I click to open/close it. I made it 1s for testing purposes so it is obvious.

css-safari-transition.mov

Copy link
Member

Choose a reason for hiding this comment

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

Made an issue for future tracking if we choose to: https://github.com/orgs/oddbird/projects/3/views/1?pane=issue&itemId=49346644

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried searching for a bug related to this but haven't been able to find anything just yet. I know it shouldn't be needed but does adding a vendor prefix change anything @stacyk ?

Copy link
Member

@stacyk stacyk Jan 9, 2024

Choose a reason for hiding this comment

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

I just tried the vendor prefix (didn't work) and I couldn't find anything either last week, but this morning I found this: https://bugs.webkit.org/show_bug.cgi?id=213349

So it looks like it was a bug, then it was fixed, and then it came back in version 16. I don't know if it is helpful to send them our use case once this is merged?

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@mirisuzanne @dvdherron Is there a bug in Safari that doesn't allow this before pseudo element to transition when we add the [open] attribute? I am able to see the transitions if I just toggle the properties (see screen recording) but not when I click to open/close it.

@stacyk Is this issue blocking merge for this PR? Or something we want to capture in a new story?

@stacyk
Copy link
Member

stacyk commented Jan 8, 2024

We shouldn't block the pr because of the transition, but i think there was a ridiculous duration and delay on that transition for testing that would need to be adjusted.

Comment on lines 35 to 40
transform: rotate(0deg);
transition: all 1s ease-out 100ms;

[open] & {
transform: rotate(90deg);
}
Copy link
Member

Choose a reason for hiding this comment

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

Made an issue for future tracking if we choose to: https://github.com/orgs/oddbird/projects/3/views/1?pane=issue&itemId=49346644

@dvdherron
Copy link
Contributor

@stacyk I think my latest changes address everything except for the possible Safari transition bug. If you want to give this a last look I think this is soon ready to merge.

Copy link
Member

@stacyk stacyk left a comment

Choose a reason for hiding this comment

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

yay! I love this app. Nice work everyone!

@stacyk stacyk merged commit d4e84f6 into main Jan 9, 2024
7 checks passed
@stacyk stacyk deleted the color-issues branch January 9, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants