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

Refactor/css modal inner #487

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Refactor/css modal inner #487

wants to merge 10 commits into from

Conversation

Sanghou
Copy link
Collaborator

@Sanghou Sanghou commented May 1, 2024

요약 *

It closes #386

스크린샷

이후 Task *

  • 작업을 쪼개서 할지 고민중이라 일부만 해서 올려보았습니다. 방향이 맞나 하는 의문이 잠깐 들어서 올렸습니다

Copy link

netlify bot commented May 1, 2024

Deploy Preview for biseo-preview ready!

Name Link
🔨 Latest commit 064ffff
🔍 Latest deploy log https://app.netlify.com/sites/biseo-preview/deploys/663b68762981df0009ad81db
😎 Deploy Preview https://deploy-preview-487--biseo-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Sanghou Sanghou changed the title [작업중] Refactor/css modal props Refactor/css modal inner May 22, 2024
@@ -1,8 +1,5 @@
import React, { type PropsWithChildren } from "react";
import {
BorderedBox,
Box,
Text,
GrayTextButton,
Scroll,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lines 329-331에 사용되는 Scroll은 deprecated component 이어서 이부분도 scroll, scrollBar을 사용하게 바꾸는 것이 좋을 것 같아요!

Comment on lines -201 to +167
<BorderedBox
w={300}
bg="gray100"
borderSize={1}
borderStyle="solid"
dir="row"
align="center"
padRight={15}
>
<div css={[w(300), bg.gray100, row, align.center, padding.right(15), border]}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 BorderedBox는 borderColor attribute가 없기 때문에 border 색깔이 default로 transparent로 설정이 되어서 그냥 평범한 Box와 appearance가 똑같아요. 그래서 line 167에서 border attribute를 빼는 게 좋을 것 같은데, 어떻게 생각하시나요?

Comment on lines -328 to +289
<Text
color="gray500"
variant="subtitle"
<p
css={[colors.gray600, text.subtitle]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 text 색깔을 gray500 에서 gray600으로 바꾼 이유가 있나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 colors => text 로 바꾸어야 할 것 같아요!

{children}
</Text>
</BorderedBox>
<p css={[colors.gray600, text.subtitle]}>{children}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

text css에서는 colors.gray600 대신에 text.gray600을 사용하여야 색깔 설정이 되는 것 같아요.

{children}
</Text>
</BorderedBox>
<p css={[colors.gray600, text.subtitle]}>{children}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 colors.gray600를 text.gray600으로 바꿔야 할 것 같아요!

@@ -220,9 +178,9 @@ const TextButton: React.FC<SubmitProps> = ({
}}
/>
<Button w={20} h={20} onClick={onSubmit}>
<Text css={[colors.blue600, text.boldtitle2, "line-height: 1"]}>+</Text>
<p css={[colors.blue600, text.boldtitle2, "line-height: 1"]}>+</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 colors.blue600를 text.blue600으로 바꿔야 할 것 같아요!

{children}
</Text>
</BorderedBox>
<p css={[colors.gray600, text.subtitle]}>{children}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 colors.gray600를 text.gray600으로 바꿔야 할 것 같아요!

{children}
</Text>
</BorderedBox>
<p css={[colors.gray600, text.subtitle]}>{children}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 colors.gray600를 text.gray600으로 바꿔야 할 것 같아요!

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.

Modal scope CSS 토큰 리팩토링
2 participants