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

Changed all label formats to match the original. #731

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

DavidJSolano
Copy link
Collaborator

This PR fixes issue #655. The new labels look like this:
Screenshot_20221120_084817
Screenshot_20221120_084831

@netlify
Copy link

netlify bot commented Nov 21, 2022

👷 Deploy request for acmcsuf pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 452f51f

@DavidJSolano DavidJSolano added the bugfix A PR that fixes a bug label Nov 25, 2022
Updated the label to contain the rel "noopener noreferrer".
Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

Nice!! Hope you had an epic Thanksgiving break 😎

src/lib/components/blog/labels.svelte Outdated Show resolved Hide resolved
src/lib/components/blog/labels.svelte Outdated Show resolved Hide resolved
src/lib/components/blog/labels.svelte Show resolved Hide resolved
src/lib/components/blog/labels.svelte Outdated Show resolved Hide resolved
src/lib/components/blog/labels.svelte Show resolved Hide resolved
All label colors were replaced with var(--acm-...)
@DavidJSolano
Copy link
Collaborator Author

Screenshot_20221206_123739
Screenshot_20221206_123754
This is how the new labels look.

Copy link
Owner

@EthanThatOneKid EthanThatOneKid left a comment

Choose a reason for hiding this comment

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

Screenshot_20221206_123739 Screenshot_20221206_123754 This is how the new labels look.

Nice! Thanks for also making sure to support dark mode :)

Copy link
Contributor

@jaasonw jaasonw left a comment

Choose a reason for hiding this comment

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

sorry, i shoulda said this earlier but can you reflect the same changes in labelfield.svelte, or maybe split the label into its own component to avoid duplication?

image

Changed the color of the labels in labelfield.svelte to match acm colors.
@DavidJSolano
Copy link
Collaborator Author

DavidJSolano commented Dec 11, 2022

Alright, I duplicated these changes to the labels in labelfield.svelte. I think it's fair to mention that the selected labels have an altered look when changed to strictly ACM colors, not sure how you guys feel about this look. Should I revert it back to the original colors?
Screenshot_20221206_011004
Let me know.

@EthanThatOneKid
Copy link
Owner

Should I revert it back to the original colors?

Thanks for following up, @DavidJSolano! I think we should go back to the original colors.

DavidJSolano and others added 4 commits December 11, 2022 18:19
I removed the ACM colors from the .selected labels and reverted them to the original blue look.
Changed labelfield.svelte color to be var(--acm--dark) to be compatible with dark/light mode.
@DavidJSolano
Copy link
Collaborator Author

Okay, this is what the final product looks like after reverting the selected labels to their original colors and making them compatible with light/dark mode.
Screenshot_20221211_063807
Screenshot_20221211_063752

src/routes/(site)/blog/labelfield.svelte Outdated Show resolved Hide resolved
Tiny syntactical error.
Copy link
Contributor

@jaasonw jaasonw left a comment

Choose a reason for hiding this comment

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

sexy

@DavidJSolano DavidJSolano merged commit 75122ec into EthanThatOneKid:main Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix A PR that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants