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

Move color issues to main section #223

Merged
merged 13 commits into from
Feb 4, 2025
Merged

Move color issues to main section #223

merged 13 commits into from
Feb 4, 2025

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Dec 10, 2024

Description

The Color Issues section is getting long, especially with #222. This moves the content to the main section, underneath the colors.

Steps to test/reproduce

The section should be collapsed on small screens by default, and expanded on large screens.

https://deploy-preview-223--oddcontrast.netlify.app/

Todo

  • Probably needs some margins or something

@jamesnw jamesnw requested a review from stacyk December 10, 2024 19:18
Copy link

netlify bot commented Dec 10, 2024

Deploy Preview for oddcontrast ready!

Name Link
🔨 Latest commit 7eae78d
🔍 Latest deploy log https://app.netlify.com/sites/oddcontrast/deploys/67a213dba4802d00082ba943
😎 Deploy Preview https://deploy-preview-223--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 linked an issue Dec 10, 2024 that may be closed by this pull request
4 tasks
* main:
  lint
  Bump the dev-minor group across 1 directory with 15 updates
  Bump jsdom from 25.0.1 to 26.0.0 in the dev-major group
  Bump the dev-minor group with 6 updates
  upgrade node and yarn
  Bump the dev-minor group with 6 updates
  Bump the dev-minor group with 7 updates
  Bump the dev-minor group with 12 updates
@jamesnw jamesnw mentioned this pull request Jan 27, 2025
1 task
@dvdherron dvdherron assigned dvdherron and unassigned mirisuzanne and stacyk Jan 28, 2025
@dvdherron dvdherron requested a review from SondraE January 28, 2025 14:18
@dvdherron
Copy link
Contributor

@SondraE @stacyk

I think this is ready for a review. I just added some spacing around the Known Issues section now that it's at the end of the main content section. And put the open triangle for the section in front of "Known Issues."

Are we okay with keeping the same colors here or do we want to make this section look different?

Played around with the width of the text. Open to adjusting as needed.

bild

bild

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.

Part of me wonders if we need to collapse this info since we are moving it to the main content area. The designer part of me thinks this whole page is getting crowded though and I want to collapse it or move it to a blog post.

Regardless, I don't think we need it in the colors selected, what about you @SondraE?

The known issues box does grow if I am on my second monitor which is taller than my laptop viewport.

Screenshot 2025-01-28 at 10 42 49 AM

src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
Copy link
Contributor

@SondraE SondraE left a comment

Choose a reason for hiding this comment

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

@dvdherron @stacyk
I agree that we don't need to keep this content in the colors selected.

I wonder about a layout design like this. Even though it's still on the same page, this is styled more like a blog post. If there are any demo images that would be useful to add into this Known Color Issues section, they could be more easily included into a layout like this.
Screenshot 2025-01-28 at 11 11 11 AM

@jamesnw
Copy link
Contributor Author

jamesnw commented Jan 28, 2025

@dvdherron @stacyk I agree that we don't need to keep this content in the colors selected.

I wonder about a layout design like this. Even though it's still on the same page, this is styled more like a blog post. If there are any demo images that would be useful to add into this Known Color Issues section, they could be more easily included into a layout like this.

This also would make it easier to link to one of the sections, as was proposed in #222.

@dvdherron
Copy link
Contributor

I wonder about a layout design like this. Even though it's still on the same page, this is styled more like a blog post. If there are any demo images that would be useful to add into this Known Color Issues section, they could be more easily included into a layout like this.

Ok, so we would change this into a section on the page with a list of issues instead of the current disclosure element? Sounds good to me. And I agree it makes sense that this section's colors don't have to match the selected colors.

@dvdherron
Copy link
Contributor

I agree that we don't need to keep this content in the colors selected.

I wonder about a layout design like this. Even though it's still on the same page, this is styled more like a blog post. If there are any demo images that would be useful to add into this Known Color Issues section, they could be more easily included into a layout like this.

@stacyk @SondraE Applied these changes in 16ee6fd and 1e6c290. Let me know what you think!

@dvdherron dvdherron requested review from SondraE and stacyk January 29, 2025 15:13
dvdherron and others added 3 commits January 29, 2025 16:16
* main:
  upgrade stylelint
  Bump the dev-minor group with 11 updates
  upgrade all deps
  Bump the dev-major group with 3 updates
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.

Left a question. I'm approving as the questions may not change anything, but there is one reordering suggestion as well. :)

src/sass/patterns/_lists.scss Outdated Show resolved Hide resolved
src/lib/components/ratio/ColorIssues.svelte Outdated Show resolved Hide resolved
src/sass/initial/_layout.scss Outdated Show resolved Hide resolved
@dvdherron dvdherron requested a review from stacyk February 4, 2025 13:19
@dvdherron
Copy link
Contributor

@stacyk Addressed your questions/comments above. This is ready for another review.

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.

👍🏻

@stacyk stacyk merged commit 6d1e4e0 into main Feb 4, 2025
7 checks passed
@stacyk stacyk deleted the color-issues-move branch February 4, 2025 21: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
Development

Successfully merging this pull request may close these issues.

Move Color Issues to main area
6 participants