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

Fix the way to calculate processedProps #283

Merged
merged 3 commits into from Aug 25, 2023

Conversation

mehm8128
Copy link
Contributor

@mehm8128 mehm8128 commented Aug 13, 2023

I found that when I use both textAlign='right'and marginRight={4}, the former is not applied.
For example:
The below code, aaa and bbb is right-aligned but ccc is not aligned.

import { Box } from '@kuma-ui/code'

   <div
        style={{
          textAlign: "right",
          marginRight: 4,
        }}
      >
        aaa
      </div>
      <Box textAlign="right">bbb</Box>
      <Box textAlign="right" marginRight={4}>
        ccc
      </Box>

So, I tried to fix it, but an error TypeError: compose is not a function occurred.
Could you check my code? (both whether my fix is proper fix and why this error occurs)

additional information: when I remove backgroundMappings from styleMappings, it works expectedly:thinking:
And declare it in other file and import it, this also works expectedly.

@changeset-bot
Copy link

changeset-bot bot commented Aug 13, 2023

🦋 Changeset detected

Latest commit: 98b259d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@kuma-ui/system Patch
@kuma-ui/babel-plugin Patch
@kuma-ui/compiler Patch
@kuma-ui/core Patch
@kuma-ui/vite Patch
@kuma-ui/webpack-plugin Patch
@kuma-ui/next-plugin Patch
next-app-router Patch
next Patch
react-vite-example Patch
webpack Patch
website Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kuma-ui-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 25, 2023 4:17pm

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 13, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 98b259d:

Sandbox Source
React Configuration
React Typescript Configuration

@poteboy
Copy link
Member

poteboy commented Aug 13, 2023

Thanks for bringing this up. I was able to reproduce the issue on my local environment as well.

I noticed that this issue doesn't occur with m or margin but specifically with mr and marginRight. It does seem like something related to compose.ts, but it's been 4 months since I last touched that code, so I need to refresh my memory on it 😅

I'll probably take a closer look within the next week.

@mehm8128
Copy link
Contributor Author

Thank you for response.
Yes, this bug occurs when right of margin-right and right of text-align: right is overlapping, so when I designate margin-right and justify-content: right, same issue occurs.
I'm looking forward to your review😃

@mehm8128
Copy link
Contributor Author

Hi, I investigated about this build error(compose is not a function).
I looked at built file which is emitted into packages/system/dist/chunk-*, and I found that compose function is used to define all function before its definition in the chunk file(and its definition is arrow function, so hoisting is not performed).
I don't know how to solve it, but I changed the file name from background.ts to xxx.ts, the build error went away and build succedded.

@kotarella1110
Copy link
Member

@mehm8128

So, I tried to fix it, but an error TypeError: compose is not a function occurred.

The error is caused by a circular reference.
Here is the code for the cause of the circular reference.

import { toCssUnit } from ".";

The following fix resolves this error.

import { toCssUnit } from "./toCSS";

both whether my fix is proper fix and why this error occurs

Your fix is appropriate 👍

@@ -81,7 +104,7 @@ export const compose = (...styleFunctions: StyleFunction[]): StyleFunction => {

const processedProps = Object.keys(outputProps).filter((key) =>
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- FIXME
newStyles.base.includes(`${outputProps[key as keyof StyledProps]}:`)
newStyles.base.includes(`${styleMappings[key as StyledKeyType]}:`)
Copy link
Member

Choose a reason for hiding this comment

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

good fix 🎉
It was not appropriate to use CSS values to extract processed props; it is correct to use CSS properties.

@poteboy
Copy link
Member

poteboy commented Aug 25, 2023

Thank you @kotarella1110 🙇‍♂️

@mehm8128
Copy link
Contributor Author

@kotarella1110 Thank you for your review!
I understand why the error was caused and I fixed it.
Could you review my code again?(CC: @poteboy )

Copy link
Member

@kotarella1110 kotarella1110 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! LGTM 🎉

@kotarella1110
Copy link
Member

@mehm8128 please fix lint error 🙏

Copy link
Member

@poteboy poteboy left a comment

Choose a reason for hiding this comment

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

Nice work 🙌

@kotarella1110 kotarella1110 merged commit 314096c into kuma-ui:main Aug 25, 2023
7 checks passed
@github-actions github-actions bot mentioned this pull request Aug 25, 2023
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.

None yet

3 participants