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

feat(xds)!: avoid overriding consumers theme by xds #1658

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

AdrianArenal
Copy link
Contributor

@AdrianArenal AdrianArenal commented Nov 20, 2024

Pull request template

Describe the purpose of the change, the specific changes done in detail, and the issue you have fixed.

Motivation and context

  • Dependencies. If any, specify:
  • Open issue. If applicable, link:

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that causes existing functionality to not work as expected)
  • Change requires a documentation update

What is the destination branch of this PR?

  • Main
  • Other. Specify:

How has this been tested?

Tests performed according to testing guidelines:

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review on my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

@AdrianArenal AdrianArenal force-pushed the OIM-589-fix-xds-overriding-consumer-theme branch 3 times, most recently from 0a2634f to f53ac96 Compare November 20, 2024 16:50
@@ -27,7 +26,9 @@ export default plugin.withOptions(
* @internal
*/
return function (helpers: TailwindHelpers) {
helpers.addComponents(deepMerge({}, components(helpers), options?.components?.(helpers)));
helpers.addComponents(deepMerge({}, components(helpers), options?.components?.(helpers)), {
respectPrefix: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, if the consumer wants to have a prefix on their classes, like 'mm', x classes won't add 'mm' on their class set.
x will have allways 'x-' manually prefixed

Comment on lines 4 to 5
content: ['./index.html', './**/*.vue'],
prefix: 'x-',
important: true,
theme: {
extend: {}
},
plugins: [
xTailwindCss({
components() {
return {};
},
utilities() {
return {};
},
dynamicUtilities() {
return {};
},
dynamicComponents() {
return {};
},
theme: {}
})
]
plugins: [xTailwindCss({})]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is exactly how we want to use xPlugin on motive-market project, we don't want to prefix our classes with 'x-' because we are using xds.. we use other plugins and we don't need to prefix anything to make them work

Copy link
Member

Choose a reason for hiding this comment

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

Invocation not needed. plugins: [xTailwindCss]

)
} as Config;
theme: {
x: deepMerge(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now we will have a 'x' property on the theme, so we are able there to customize the theme provided by x.
Only x classes will be affected when manipulating styles within the theme.x block.
isolation++ 👍🏽

Copy link
Member

Choose a reason for hiding this comment

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

You also need to reference the default theme of the plugin to allow Tailwind generates the property identifiers like bg, text, etc. with the default theme. The theme under x is only used to generate the custom properties in the plugin.

function (options = {}) {
  const customXTheme = deepMerge(
    { colors: { current: 'currentColor', transparent: 'transparent' } },
    xTheme,
    options.theme
  );
  return {
    theme: {
      extend: customXTheme,
      x: customXTheme
    }
  };
}

Remember to replace all x-bg-... for bg-... (some are still pending)

Copy link
Contributor

Choose a reason for hiding this comment

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

if we add the customXTheme to the extend field... aren't we still overriding the default values from the theme with the ones from the x plugin? 🤔

Comment on lines -10 to -26
plugins: [
xTailwindCss({
components() {
return {};
},
utilities() {
return {};
},
dynamicUtilities() {
return {};
},
dynamicComponents() {
return {};
},
theme: {}
})
]
Copy link
Contributor

@annacv annacv Nov 21, 2024

Choose a reason for hiding this comment

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

Hi, if I am not wrong, maybe we should keep this part to be able to access to components() within the setups 👀 , I mean ok remove prefix, but I would leave the part regarding the plugins section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm out of context and I didn't probably understand that,
You mean you need this block to do setups? for what purpose ? Could you give me an example ? 🙏🏽

Copy link
Member

@joseacabaneros joseacabaneros left a comment

Choose a reason for hiding this comment

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

Try this regex (theme\(')[^x] to look for theme uses without x.
Screenshot 2024-11-21 at 12 55 37

Comment on lines 4 to 5
content: ['./index.html', './**/*.vue'],
prefix: 'x-',
important: true,
theme: {
extend: {}
},
plugins: [
xTailwindCss({
components() {
return {};
},
utilities() {
return {};
},
dynamicUtilities() {
return {};
},
dynamicComponents() {
return {};
},
theme: {}
})
]
plugins: [xTailwindCss({})]
Copy link
Member

Choose a reason for hiding this comment

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

Invocation not needed. plugins: [xTailwindCss]

packages/x-tailwindcss/src/x-tailwind-plugin/components.ts Outdated Show resolved Hide resolved
@AdrianArenal AdrianArenal force-pushed the OIM-589-fix-xds-overriding-consumer-theme branch from a091841 to c330593 Compare November 25, 2024 15:25
@AdrianArenal AdrianArenal marked this pull request as ready for review November 25, 2024 15:30
@AdrianArenal AdrianArenal requested a review from a team as a code owner November 25, 2024 15:30
@AdrianArenal AdrianArenal force-pushed the OIM-589-fix-xds-overriding-consumer-theme branch from c330593 to 35bfb5e Compare November 25, 2024 15:41
@AdrianArenal AdrianArenal force-pushed the OIM-589-fix-xds-overriding-consumer-theme branch from 35bfb5e to 39282e5 Compare November 25, 2024 15:41
@AdrianArenal AdrianArenal force-pushed the OIM-589-fix-xds-overriding-consumer-theme branch from 39282e5 to 7397219 Compare November 25, 2024 15:52
@diegopf diegopf merged commit bd3ed79 into main Nov 25, 2024
4 checks passed
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.

4 participants