-
Notifications
You must be signed in to change notification settings - Fork 23
Temporary Tailwind4 migration for MotiveMarket #1950
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
base: main
Are you sure you want to change the base?
Conversation
| <span class="text-accent-50 font-bold">{{ resultsLength }}</span> | ||
| <span>of</span> | ||
| <span class="x-text-accent-50">{{ totalResults }}</span> | ||
| <span class="text-accent-50">{{ totalResults }}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text-accent is from the x-tailwindcss should keep be with x-, right? same for x-font-bold etc etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but not. Both classes are generated from x-tailwindcss but they are from tailwind.
font-bold: https://tailwindcss.com/docs/font-weight
text-{color}-{tone}: https://tailwindcss.com/docs/color
victorcg88
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job Fran!!!
| color: shadeColor, | ||
| })), | ||
| { prefix: '&-' }, | ||
| return mapColorsFlat((color, colorName) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return mapColorsFlat((color, colorName) => { | |
| return mapColors((color, colorName) => { |
| ) | ||
| } | ||
|
|
||
| export function mapColorsFlat<T extends CssStyleOptions>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export function mapColorsFlat<T extends CssStyleOptions>( | |
| export function mapColors<T extends CssStyleOptions>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to know why two different functions mapColors and mapColorsFlat are needed for the migration
| export interface CSSRuleObject { | ||
| [selector: string]: CSSRuleValue | string | number | undefined | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| export interface CSSRuleObject { | |
| [selector: string]: CSSRuleValue | string | number | undefined | |
| } |
This should be in the type.ts right?
| @config './tailwind.config.ts'; | ||
|
|
||
| @plugin "@empathyco/x-tailwindcss/plugin"; | ||
| @plugin "@empathyco/x-tailwindcss/old-ds-plugin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get rid of this:
| @plugin "@empathyco/x-tailwindcss/old-ds-plugin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this? If something is needed (Variables or whatever), move it where necessary. x-components or x-tailwind-plugin.
1c5747f to
07a1e4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't change the CHANGELOG to know what we have in the past
| "moduleResolution": "node", | ||
| "types": ["tailwindcss"], | ||
| "paths": { | ||
| "tailwindcss/plugin": ["./node_modules/tailwindcss/dist/plugin.d.mts"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explain this change ?
| "@rollup/plugin-commonjs": "29.0.0", | ||
| "@tailwindcss/postcss": "4.1.17", | ||
| "@tailwindcss/vite": "4.1.17", | ||
| "@types/tailwindcss": "^3.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "@types/tailwindcss": "^3.1.0", | |
| "@types/tailwindcss": "3.1.0", |
| vueDocsPlugin, | ||
| Inspector(), | ||
| tailwindcss({ | ||
| config: resolve(__dirname, './tailwind.config.ts'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was moved to src/tailwind/tailwind.config.ts right?
| @@ -0,0 +1,4 @@ | |||
| @import 'tailwindcss'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @import 'tailwindcss'; | |
| @import 'tailwindcss' prefix(xds); |
This tailwind demo is not only for showcase. It is also a development tool for setups. Usually, a developer runs this project, chooses a styling to apply, and copies the whole bunch of classes. There is a feature to copy styles from the demo.
With this prefix change, this feature becomes useless, as we would need to add the prefix manually again, which will be the case for 99.9999% of the cases.
| @config './tailwind.config.ts'; | ||
|
|
||
| @plugin "@empathyco/x-tailwindcss/plugin"; | ||
| @plugin "@empathyco/x-tailwindcss/old-ds-plugin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we remove this? If something is needed (Variables or whatever), move it where necessary. x-components or x-tailwind-plugin.
| ...badgeBright(helpers), | ||
| }, | ||
| { | ||
| prefix: '.x-badge-', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| prefix: '.x-badge-', | |
| prefix: '.badge-', |
Shouldn't be like that?
| }, | ||
| { | ||
| prefix: '&-', | ||
| prefix: '&.input-', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| prefix: '&.input-', | |
| prefix: '.input-', |
Right?
| { | ||
| ...textSizes(helpers), | ||
| }, | ||
| { prefix: '&.text1-' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
| { | ||
| ...textSizes(helpers), | ||
| }, | ||
| { prefix: '&.text2-' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
| { prefix: '&-' }, | ||
| { prefix: '.layout-' }, | ||
| ), | ||
| ...minMargin(helpers), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why minMargin and maxWidth now are out of rename?
| ) | ||
| } | ||
|
|
||
| export function mapColorsFlat<T extends CssStyleOptions>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to know why two different functions mapColors and mapColorsFlat are needed for the migration
| }, | ||
| ) | ||
|
|
||
| const xTailwindPlugin: unknown = _xTailwindPlugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why export something unknown?
| "main": "dist/cjs/index.js", | ||
| "module": "dist/esm/index.js", | ||
| "types": "dist/types/index.d.ts", | ||
| "exports": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the export process now have to be done this way?
| @@ -0,0 +1,4 @@ | |||
| @import 'tailwindcss'; | |||
|
|
|||
| @plugin "../../../src/x-tailwind-plugin/plugin"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remains tailwind.config.ts in the demo root with plugins: [xTailwindCss],. Are both needed?
| <BaseEventButton | ||
| :events="events" | ||
| class="x-events-modal-close-button x-button" | ||
| class="x-events-modal-close-button xds:button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using both prefix x and xds is something that drives me crazy 😅
I know the reason behind but I don't like the result mix
|
|
||
| <!-- Facets --> | ||
| <Facets class="x-gap-24"> | ||
| <Facets class="gap-24"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be xds: prefixed?
| @@ -0,0 +1,4 @@ | |||
| @import 'tailwindcss'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefix the demo too to make it more realistic since supposedly it will to be the way we will use the XDS in consumers
| * @public | ||
| * The helpers provided internally by our Tailwind plugin. | ||
| * Tailwind CSS 4 no longer exposes `PluginAPI`, | ||
| * so we recreate the pieces we need. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment explains very well that we're giving Tailwind 3 a facelift to make it look like Tailwind 4. From my point of view, it's not really a migration.
# Conflicts: # packages/x-components/package.json # packages/x-tailwindcss/package.json # pnpm-lock.yaml
438933c to
dd29ddb
Compare
dd29ddb to
4fb3dbe
Compare
Pull request template
Describe the purpose of the change, the specific changes done in detail, and the issue you have fixed.
Motivation and context
Type of change
What is the destination branch of this PR?
MainHow has this been tested?
Tests performed according to testing guidelines:
Checklist: