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/BAR-165] 끄적이는 레이아웃 작성 & 데이터 구조화 #38

Merged
merged 14 commits into from
Jan 23, 2024

Conversation

wonjin-dev
Copy link
Member

@wonjin-dev wonjin-dev commented Jan 14, 2024

Summary

구현 내용 및 작업한 내역을 요약해서 적어주세요

스크린샷 2024-01-24 오전 1 38 00 스크린샷 2024-01-24 오전 2 03 21
  • feature → domain 이름 변경
    • alias 추가
  • max-width 전역 스타일링 적용
  • 끄적이는 레이아웃 / 데이터 플로우 설계
  • 컴포넌트 스타일링

To Reviewers

PR을 볼 때 주의깊게 봐야하거나 말하고 싶은 점을 적어주세요

  • feature에서 domain으로 이름을 변경하였습니다.
  • body에 전역 스타일링을 적용했습니다 margin: 0 auto & max-width

How To Test

PR의 기능을 확인하는 방법을 상세하게 적어주세요

/write페이지에서 확인 가능

@wonjin-dev wonjin-dev changed the base branch from main to feature/BAR-162 January 15, 2024 15:43
@wonjin-dev wonjin-dev changed the base branch from feature/BAR-162 to main January 15, 2024 15:44
@wonjin-dev wonjin-dev changed the base branch from main to feature/BAR-162 January 15, 2024 15:45
@wonjin-dev wonjin-dev changed the base branch from feature/BAR-162 to main January 15, 2024 15:46
@wonjin-dev wonjin-dev marked this pull request as ready for review January 15, 2024 16:02
Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

수고하셨습니당 👍🏻

Comment on lines 18 to 25
export const value = style({
marginBottom: '16px',
color: COLORS['Grey/900'],
fontSize: '15px',
fontWeight: '400',
lineHeight: '24px',
letterSpacing: '-0.2px',
});
Copy link
Member

Choose a reason for hiding this comment

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

export const content = style([
  sprinkles({ typography: '15/Body/Regular' }),
  {
    ...
  },
]);

로 사용하면 될 것 같습니다!

Comment on lines 18 to 24
export const dateLabelWrapper = style({
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
marginTop: '12px',
marginBottom: '24px',
});
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
export const dateLabelWrapper = style({
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
marginTop: '12px',
marginBottom: '24px',
});
export const dateLabelWrapper = style([
utils.flexCenter,
{
marginTop: '12px',
marginBottom: '24px',
}
]);

이렇게 사용하셔도 됩니다!

Comment on lines 26 to 33
export const dateLabel = style({
padding: '6px 28px',
display: 'flex',
justifyContent: 'center',
alignItems: 'center',
border: `1px solid ${COLORS['Grey/200']}`,
borderRadius: '100px',
});
Copy link
Member

Choose a reason for hiding this comment

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

utils.flexJustifyCenter도 있습니다!


import { type WriteHisotry } from '../types';
import WriteHistoryCard from './components/Card';
import * as style from './index.css';
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
import * as style from './index.css';
import * as styles from './style.css';

엇 이렇게 작성하는 걸로 통일했던 거 같아요.

Comment on lines 12 to 16
export const contentWrapper = style({
display: 'grid',
gridTemplateColumns: '1fr 1fr 1fr',
gap: '16px',
});
Copy link
Member

@dmswl98 dmswl98 Jan 15, 2024

Choose a reason for hiding this comment

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

image

위 레이아웃을 위해 grid-auto-flow 속성도 넣어야 할 것 같습니당

@@ -20,7 +20,7 @@
"@assets/*": ["./src/assets/*"],
"@components/*": ["./src/components/*"],
"@constants/*": ["./src/constants/*"],
"@feature/*": ["./src/feature/*"],
"@domain/*": ["./src/domain/*"],
Copy link
Contributor

Choose a reason for hiding this comment

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

@wonjin-dev
요건 어떤 이유로 수정된걸까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

domain 하위에 각각의 feature 폴더들이 들어가야하는건지? 궁금했습니다.

Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

수고하셨슴니당

Copy link
Contributor

@miro-ring miro-ring left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

@wonjin-dev wonjin-dev merged commit 145fe68 into main Jan 23, 2024
@wonjin-dev wonjin-dev deleted the feature/BAR-165 branch January 23, 2024 17:04
@wonjin-dev
Copy link
Member Author

@dongkyun-dev @dmswl98

추가 커밋이 있어서 노티드립니다~ 리뷰 주시면, API 연결 + 페이징 처리하면서 함께 반영할게요 !

  • 추가로 동균님 레이아웃 머지되면 바로 반영하겠습니다 ~

Copy link
Member

@dmswl98 dmswl98 left a comment

Choose a reason for hiding this comment

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

굿굿~~ 수고하셨습니당 👍🏻

Comment on lines +16 to +33
const parser = (arr: Write[]): Write[][] => {
let queue1: Write[] = [];
let queue2: Write[] = [];
let queue3: Write[] = [];

arr.forEach((history, i) => {
const extra = i % 3;
if (extra === 0) {
queue1.push(history);
} else if (extra === 1) {
queue2.push(history);
} else {
queue3.push(history);
}
});

return [queue1, queue2, queue3];
};
Copy link
Member

Choose a reason for hiding this comment

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

컴포넌트 내부말고 유틸 함수로 빼도 좋을 것 같아요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants