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 token-based CSS theming #11262 #11319

Merged
merged 22 commits into from
Oct 7, 2024
Merged

Conversation

jacobtylerwalls
Copy link
Member

@jacobtylerwalls jacobtylerwalls commented Aug 9, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Begin building a theme preset for arches that can be used/extended in arches applications also.

PrimeVue docs

Issues Solved

Closes #11262

Checklist

  • I targeted one of these branches:
    • dev/8.0.x
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Accessibility Checklist

Developer Guide

Topic Changed Retested
Color contrast
Form fields
Headings
Links
Keyboard
Responsive Design
HTML validation
Screen reader

Ticket Background

  • Sponsored by: Getty Conservation Institute
  • Found by: @
  • Tested by: @
  • Designed by: @

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

Nice! Exciting to start getting theming buckled down

arches/app/media/js/utils/create-vue-application.js Outdated Show resolved Hide resolved
arches/app/media/js/utils/create-vue-application.js Outdated Show resolved Hide resolved
@jacobtylerwalls jacobtylerwalls linked an issue Aug 12, 2024 that may be closed by this pull request
@chrabyrd
Copy link
Contributor

My assumption is that basic theme-switching will be stubbed out here, but lemme know if that assumption is wrong 👍

@jacobtylerwalls
Copy link
Member Author

I think there's value in doing that in follow-up work in another PR, as theme switching isn't really blocking further arches-references development, but having this in is.

red: palette(archesColors.red),
},
semantic: {
primary: palette(archesColors.blue),
Copy link
Member Author

Choose a reason for hiding this comment

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

This avoids having to manually override every sky token. On our side, we just have to define one blue color and let primevue deal with gradients.

arches/app/src/arches/themes/default.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

Overall looks good and I think there's just a few large-stroke things to hash out

arches/app/src/arches/themes/default.ts Show resolved Hide resolved
},
color: "{arches.legacy.sidebar}",
},
iconSize: "small",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend remove this, I don't believe we should have our "default" icon size be small ( or that we should offer a "default" icon size)

Copy link
Member Author

Choose a reason for hiding this comment

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

PrimeVue themes come with an iconSize whether we like it or not (1rem), so I have a feeling we will want to override it with something. But we can definitely punt.

arches/app/src/arches/themes/default.ts Outdated Show resolved Hide resolved
},
semantic: {
primary: palette(archesColors.blue),
navigation: {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this needs to either be removed or have more descriptive naming. arches-navigation-list could refer to many things. arches-navigation-list-padding is better. same with color IMO but I feel less strongly there

Copy link
Member Author

Choose a reason for hiding this comment

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

This is another one that comes from primevue theming. (Sorry, tried to clarify this by putting "custom" tokens beneath a comment, but as of now we won't have any custom tokens.) This is what we get if we do nothing:

navigation
  item
    {padding: '0.5rem 0.75rem', borderRadius: '{border.radius.sm}', gap: '0.5rem'}
  list 
    {padding: '0.25rem 0.25rem', gap: '2px'}
  submenuIcon
    {size: '0.875rem'}
  submenuLabel
    {padding: '0.5rem 0.75rem', fontWeight: '600'}

I think leaving this in is less opinionated by taking some of these paddings down to 0 or at least equal X/Y


On second look, the navigation-color one is custom, so I'll find a way to rename/clarify.

arches/app/src/arches/themes/default.ts Show resolved Hide resolved
@jacobtylerwalls
Copy link
Member Author

@chrabyrd No doubt we'll want to evaluate those navigation tokens again with the benefit of some usage experience, but I think this is ready for another look!

Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@jacobtylerwalls jacobtylerwalls merged commit a8e9783 into dev/8.0.x Oct 7, 2024
6 checks passed
@jacobtylerwalls jacobtylerwalls deleted the jtw/arches-theme branch October 7, 2024 21:37
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.

Create Arches PrimeVue4 token-based theming pattern
2 participants