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

Feature/button,icon,typography prop업데이트 #56

Merged
merged 18 commits into from
Jul 25, 2021

Conversation

Hemistone
Copy link
Collaborator

@Hemistone Hemistone commented Jul 23, 2021

테크스펙

변경사항

Typography

  • Heading에 존재하던 margin-bottom을 0으로 변경

Icon

  • Icon size가 sm, md, lg로 3단계가 되었습니다.

Button

  • button type: line이 없어졌습니다.
  • button size가 sm, md, lg로 3단계가 되었습니다.
  • button에 disabled, loading시 터치가 되지 않는 Unit Test를 추가하였습니다.

사전 확인사항

  1. Button Unit Testing 도중 onPress를 원천차단하지 않으면 jest가 동작하지 않는 현상이 있어, 아래와 같이 핸들링하였습니다.
// 알수없는 이유로 onPress를 disabled일 경우 없애지 않으면 테스트 코드가 안돌아감
<SPressable
  onPress={!(props.disabled || props.loading) && props.onPress}
  disabled={props.disabled || props.loading}
>
  1. Pressable 컴포넌트에서 모든 Container Styling을 집어넣는 것을 시도해봤으나, 이슈가 있어 작동에 문제가 있었습니다. 복잡한 Pressable을 따로 정의해서 사용하면 가능은 해보이나, 불필요한 작업으로 생각되어 ContainerPressable을 분리하여 적용했습니다.
    [React Native] Pressable styling styled-components/styled-components#3228.

@Hemistone Hemistone requested a review from GwangjoGong July 23, 2021 11:15
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #56 (bfcf6c9) into develop (7956d88) will increase coverage by 1.57%.
The diff coverage is 75.51%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #56      +/-   ##
===========================================
+ Coverage    74.92%   76.49%   +1.57%     
===========================================
  Files           47       47              
  Lines          662      668       +6     
  Branches       113      106       -7     
===========================================
+ Hits           496      511      +15     
+ Misses          77       75       -2     
+ Partials        89       82       -7     
Impacted Files Coverage Δ
src/atoms/Typography/Typography.tsx 75.00% <ø> (ø)
src/atoms/Icon/Icon.tsx 69.23% <66.66%> (+6.73%) ⬆️
src/atoms/Button/Button.tsx 80.70% <76.74%> (+18.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c0e2fa...bfcf6c9. Read the comment docs.

Copy link
Contributor

@GwangjoGong GwangjoGong left a comment

Choose a reason for hiding this comment

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

Button, Icon 관련 리뷰 남겼습니다.

return `width: 20px; height: 20px;`;
case 'lg':
return `width: 32px; height: 32px`;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

P4: 개인적으로 case: 'md'까지 switch로 다뤘으면 좋겠습니다

disabledColor,
...props
}) => {
let textColor: CustomColors['textColor'];
Copy link
Contributor

Choose a reason for hiding this comment

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

P4: 함수로 너무 묶고싶습니다...

import _ from 'lodash';
import colors from '../../styles/colors';

// default color : opacity value change
Copy link
Contributor

Choose a reason for hiding this comment

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

P4: Button 디렉토리 내에 정의되어 있고, Button 디렉토리 내에서만 사용되므로 파일명은 colors.ts, 타입 및 변수 명들도 button prefix를 떼도 괜찮지 않을까요?

Copy link
Contributor

@GwangjoGong GwangjoGong left a comment

Choose a reason for hiding this comment

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

GTG

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.

3 participants