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

[core] Fix useTheme import change #134

Merged
merged 3 commits into from
Jun 10, 2024
Merged

Conversation

brijeshb42
Copy link
Contributor

  • Ignore transforming useTheme reference
  • Fix the path to generate theme source

Fixes #124

* Ignore transforming useTheme reference
* Fix the path to generate theme source
@brijeshb42 brijeshb42 added the bug 🐛 Something doesn't work label Jun 10, 2024
import { expect } from 'chai';
import createExtendSxProp from '../../src/createExtendSxProp';

describe('createExtendSxProp', () => {
it('return the new copy of input', () => {
const original = { color: 'red' };
expect(createExtendSxProp()(original)).to.not.equal(original);
expect(createExtendSxProp()(original)).to.equal(original);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siriwatknp Hopefully this is correct.

Copy link
Member

Choose a reason for hiding this comment

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

expect(createExtendSxProp()(original)).to.not.equal(original); is already correct (at least, it's my intention) to ensure that the result creates a new object the same as what extendSxProp does.

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 changed it because it was failing.

function extendSxProp(props) {
  return props;
}

It's returning the same object. So the reference won't be different.

Copy link
Member

Choose a reason for hiding this comment

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

Opps, then it should be the implementation that should be fixed.

function extendSxProp(props) {
  return { ...props };
}

import { useTheme } from '../zero-styled';
import { useTheme } from '@pigment-css/react';

console.log(useTheme);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(useTheme);

Copy link
Member

Choose a reason for hiding this comment

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

Is the console.log intentional?

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 part of the test to make sure we don't have the same issue that this PR fixes.

Copy link
Member

Choose a reason for hiding this comment

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

I see, got it. Can you add a comment that it's intentional? I am sure others will ask if they see console.log

@@ -347,7 +347,10 @@ export const plugin = createUnplugin<PigmentOptions, true>((options) => {
if (id.endsWith('styles.css')) {
return theme ? generateTokenCss(theme) : _code;
}
if (id.includes('pigment-css-react/theme')) {
if (
id.includes('pigment-css-react/theme') ||
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what's pigment-css-react/theme is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point, it was for local testing in workspaces. We can maybe clean this up separately.

@@ -1,11 +1,10 @@
import { describe } from 'yargs';
Copy link
Member

Choose a reason for hiding this comment

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

😢 my bad.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Appreciate the fix!

@brijeshb42 brijeshb42 merged commit 9da7690 into mui:master Jun 10, 2024
11 of 12 checks passed
@brijeshb42 brijeshb42 deleted the fix/usetheme branch June 10, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[nextjs] useTheme replacement does not work
2 participants