Investigate migrating XBuilder frontend styling to Tailwind CSS#2986
Investigate migrating XBuilder frontend styling to Tailwind CSS#2986nighca wants to merge 1 commit intogoplus:devfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses the investigation into adopting Tailwind CSS for the XBuilder frontend. It includes a comprehensive assessment document that evaluates the feasibility and impact of such a migration, ultimately recommending a cautious, incremental hybrid strategy. The changes also lay the groundwork for this strategy by integrating Tailwind CSS into the build system and refactoring a selection of UI components to utilize Tailwind's utility-first approach, thereby providing a practical pilot for future adoption. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates Tailwind CSS into the spx-gui frontend, including adding new dependencies, configuring Vite, and introducing a dedicated tailwind.css file for custom theme variables and utilities. Several Vue components have been refactored to replace their SCSS styling with Tailwind utility classes. The review feedback highlights opportunities to improve the Tailwind implementation by adopting more idiomatic practices, such as using responsive utility classes directly in templates for dynamic widths and refining the Tailwind theme variable definitions for font sizes and line-heights to align with standard conventions.
| <script setup lang="ts"> | ||
| import { computed } from 'vue' | ||
| import { useResponsive } from '@/components/ui' | ||
|
|
||
| // different size of centered-content for different pages | ||
| type Size = 'medium' | 'large' | ||
|
|
||
| withDefaults( | ||
| const props = withDefaults( | ||
| defineProps<{ | ||
| size?: Size | ||
| }>(), | ||
| { | ||
| size: 'medium' | ||
| } | ||
| ) | ||
| </script> | ||
|
|
||
| <style scoped lang="scss"> | ||
| @import '@/components/ui/responsive'; | ||
| const isDesktopLarge = useResponsive('desktop-large') | ||
|
|
||
| .centered { | ||
| position: relative; | ||
| margin-left: auto; | ||
| margin-right: auto; | ||
| const width = computed(() => { | ||
| if (props.size === 'large') return isDesktopLarge.value ? '1492px' : '1240px' | ||
| return isDesktopLarge.value ? '1240px' : '988px' | ||
| }) | ||
|
|
||
| &.size-medium { | ||
| width: 988px; | ||
| @include responsive(desktop-large) { | ||
| width: 1240px; | ||
| } | ||
| } | ||
| const centeredStyle = computed(() => ({ | ||
| width: width.value | ||
| })) | ||
| </script> | ||
|
|
||
| &.size-large { | ||
| width: 1240px; | ||
| @include responsive(desktop-large) { | ||
| width: 1492px; | ||
| } | ||
| } | ||
| } | ||
| </style> | ||
| <template> | ||
| <section class="relative mx-auto self-center" :style="centeredStyle"> | ||
| <slot></slot> | ||
| </section> | ||
| </template> |
There was a problem hiding this comment.
While this refactoring successfully removes the SCSS, it introduces styling logic into the <script> section and uses inline styles. A more idiomatic Tailwind CSS approach would be to use responsive utility classes directly in the template.
This would involve:
- Defining your custom
desktop-largebreakpoint in your Tailwind configuration (tailwind.cssortailwind.config.js). - Using dynamic classes in the template to apply the correct widths based on screen size and the
sizeprop.
This keeps your styling declarative and closer to the HTML structure, which is one of the main benefits of Tailwind CSS.
Here's how you could configure the breakpoint in tailwind.css:
@theme {
/* ... other theme settings */
--breakpoint-desktop-large: 1680px;
}And here is a suggested implementation for this component:
<script setup lang="ts">
import { computed } from 'vue'
// different size of centered-content for different pages
type Size = 'medium' | 'large'
const props = withDefaults(
defineProps<{
size?: Size
}>(),
{
size: 'medium'
}
)
const sizeClasses = computed(() => {
if (props.size === 'large') {
return 'w-[1240px] desktop-large:w-[1492px]'
}
return 'w-[988px] desktop-large:w-[1240px]'
})
</script>
<template>
<section class="relative mx-auto self-center" :class="sizeClasses">
<slot></slot>
</section>
</template>
| --text-ui-15: 15px; | ||
| --text-ui-15--line-height: 1.5; | ||
| --text-ui-20: 20px; | ||
| --text-ui-20--line-height: 1.5; |
There was a problem hiding this comment.
The theme variables for font sizes should be prefixed with --font-size-, not --text-. Also, defining line-height separately with a --text-*-line-height variable is not a standard Tailwind practice and likely won't work as expected. It's better to define the font size and its default line-height together in a tuple.
This makes your utility classes more concise. Instead of needing to add a separate class for line-height, you can just use text-ui-15 and have both properties applied.
| --text-ui-15: 15px; | |
| --text-ui-15--line-height: 1.5; | |
| --text-ui-20: 20px; | |
| --text-ui-20--line-height: 1.5; | |
| --font-size-ui-15: [15px, 1.5]; | |
| --font-size-ui-20: [20px, 1.5]; |
1 participant
close #2981