refactor: css mixins and custom-classes to tailwind #1138
Conversation
| const tailwindSize = computed(() => ({ | ||
| 'size-3': size === 'xsmall', | ||
| 'size-4': size === 'small', | ||
| 'size-5.5': size === 'medium', |
There was a problem hiding this comment.
Keeping this the same as before results in an odd number. We might re-visit it later.
| <style> | ||
| @reference '@opencloud-eu/design-system/tailwind'; | ||
| @layer utilities { | ||
| .create-and-upload-actions .oc-drop .oc-icon svg { | ||
| /* reset the resource icon height because the ResourceIcon component messes with it */ | ||
| @apply h-5.5; | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
This is super messed up. All because the ResourceIcon component reduces the file icons height to 70%, which leads to the icons in this component to be distorted, because they sit in a container with h-full. Which is necessary because we want the icons to have some custom size here... We should probably clean that up at some point.
JammingBen
force-pushed
the
refactor/css-mixins-to-tailwind
branch
from
e7cf57b to
7579ecb
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors CSS mixins and custom classes to use Tailwind utility classes, streamlining the design system by consolidating styling approaches. The refactoring removes custom mixins that used arbitrary calculations and replaces them with consistent Tailwind spacings and sizes.
- Migrates button, icon, and form component sizing from custom mixins to Tailwind classes
- Replaces custom justify-content classes with native Tailwind utilities
- Updates snapshot tests to reflect the new class structure
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/design-system/src/helpers/types.ts | Adds JustifyContentType for type safety |
| packages/design-system/src/helpers/tailwind.ts | New utility functions for Tailwind class mappings |
| packages/design-system/src/components/OcButton/OcButton.vue | Refactors to use Tailwind classes instead of custom CSS |
| packages/design-system/src/components/OcIcon/OcIcon.vue | Removes custom size mixins, uses Tailwind size classes |
| packages/design-system/src/components/OcCheckbox/OcCheckbox.vue | Migrates from custom size classes to Tailwind |
| packages/design-system/src/components/OcRadio/OcRadio.vue | Similar migration to Tailwind size utilities |
| Multiple test snapshot files | Updates expected output to match new class structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
We must not interpolate tailwind css classes because we would risk them not getting included in the final bundle.
JammingBen
force-pushed
the
refactor/css-mixins-to-tailwind
branch
from
7579ecb to
b198273
Compare
| class="m-0.5 border-2 border-role-outline outline-0 focus-visible:outline outline-role-secondary" | ||
| class="oc-checkbox m-0.5 border-2 border-role-outline outline-0 focus-visible:outline outline-role-secondary rounded-sm checked:bg-white disabled:bg-role-surface-container-low indeterminate:bg-white bg-transparent inline-block overflow-hidden" | ||
| :class="{ | ||
| 'oc-checkbox-checked bg-white': isChecked, |
There was a problem hiding this comment.
is it doing the same that we have bg-white when isChecked is true and checked:bg-white in the static class list?
There was a problem hiding this comment.
The last time I checked/tried, isChecked and the native :checked behaved a bit different for some reason, so I kept both 🤷 Didn't dive too deep into the implementation though.
…lwind refactor: css mixins and custom-classes to tailwind
This is not a necessary, but IMO nice-to-have adjustment. The mixins don't really comply with Tailwind spacings/sizes and do calculations with random numbers instead. So I figured it makes sense to migrate them to proper Tailwind classes. It's also easier to read and understand. The only exception is the button appearance and color mixin. I haven't decided whether we should keep it or not, but for now I think it's fine to have it.
Also removes some of the custom classes (like e.g.
oc-button-justify-content) and replaces them with Tailwind classes. IMO this is more streamlined and easy to read, since we can (and should) handle almost all CSS within an element'sclass(or:class) tag.Also removes the Tailwind class interpolation again. We must not interpolate Tailwind CSS classes because we would risk them not getting included in the final bundle.
tl;dr: This PRs aims to unify and streamline the styling of components in the design system by using Tailwind classes via the
classattribute whenever possible. No more styling all over the place.refs #937