feat: add step component #76

yolophg · 2024-06-25T20:21:10Z

Checklist before merging

Link an issue with the pull request
Ensure no errors or warnings on the browser console
Avoid additional major pushes after approval (if necessary, request a new review)

index.html

components/step.js

sounmind · 2024-07-03T21:10:34Z

components/step.js

+  get requiredAttributes() {
+    return ["link"];
+  }


이 getter는 어디에서 사용되는 걸까요??

오 여기에서 사용되고 있군요

만약 attribute로 들어온 link가 유효한 형식인지 확인하려면 즉, 해당 컴포넌트에 특별한 추가 검증 논리가 필요하다면,
여기서 어떻게 구현하면 되나요?

아니면 단순히 attribute가 들어왔는지 안 들어왔는지만 검사하는 용도의 구현일까요?

네, 후자의 용도로 구현하였습니다. 내부 컴포넌트가 하나 더 생기면서 아래와 같이 attribute가 있는지 없는지 검사하는 로직이 반복되어서 하나의 validateAttributes에서 처리해줄 수 있는 용도로 구현해보았습니다.

if (!this.hasAttribute("link")) { throw new Error('The "link" attribute is required.'); }

들어온 link가 유효한 형식인지 검증해주는 부분까지는 사실 깊게 생각을 못했는데, 필요하다면 link 유효성 검증 로직을 새로 추가하여 구현할 수 있을 것 같아요!

@yolophg 님, 우리 게터 패턴 안 쓰기로 하지 않았나요? 저도 @sounmind 님 처럼, 한 참 해맸습니다 ㅋㅋㅋ DRY한 코드도 물론 좋지만 저는 개인적으로 중복이 좀 있더라도 읽기 쉬운 코드가 더 좋은 것 같습니다!

아 기억나네요. 문서화해두진 않았었나요? #44
BaseClass가 전부에게 익숙하지 않으면 이해하는데 시간이 꽤 걸린다는 게 단점이긴 하네요.

sounmind · 2024-07-03T23:47:21Z

@yolophg 지금 한 PR에 16개의 커밋이 있고 그 중에 Merge commit은 3개나 되는데 제가 느끼기에는 좀 과한 것 같습니다. 혹시 commit들을 정리해주실 수 있을까요? 최소한 Merge commit만 정리해도 될 것 같습니다~!! 정리할 수 없다면 말씀해주세요.

components/step.js

DaleSeo · 2024-07-04T00:18:02Z

components/step.js

@@ -0,0 +1,132 @@
+import { css, html } from "../html-css-utils.js";
+
+class BaseStepElement extends HTMLElement {


@sounmind 님, 이렇게 Base 클래스를 만들어서 Boilerplate 코드를 최소화하는 것에 대해서 지난 주에 우리도 잠깐 논의했었는데 @yolophg 님께서 벌써 이렇게 해보셨요 ㅋㅋ 저는 개인적으로 이렇게 컴포넌트 별로 하는 것보다는, 차라리 css, html처럼 전역에서 제공하는 것도 나쁘지 않겠다는 생각이 드네요.

섣부른 추상화가 될까봐 경계하기는 하는데, 조금씩 논의해보면 좋을 것 같아요!

@sounmind 역시 우리 테크리드의 깊은 고찰에 감탄합니다 👍

components/step.js

DaleSeo · 2024-07-04T00:26:31Z

components/step.js

+      <article class="step">
+        <section class="step-title">
+          <h3>Step ${step}</h3>
+        </section>
+        <img class="step-icon" src="${iconSrc}" alt="Step icon" />
+        <section class="step-content">
+          <slot name="content"></slot>
+        </section>
+      </article>


저희가 논의한 컴포넌트 API 디자인과 잘 부합하는 구현인 것 같습니다! 💯

DaleSeo

사소한 추가 코멘트가 있지만 전반적으로 완성이 된 것 같아서 승인합니다. @sounmind 님께서, #76 (comment) 에서 피드백 드렸던 것처럼, PR의 커밋 이력을 좀 더 깔끔하게 관리하시면 팀원 간에 코드 리뷰 경험을 개선하는데 큰 도움이 될 거라 생각합니다.

components/step-section.js

yolophg · 2024-07-08T18:31:49Z

Step section, list 컴포넌트 포함

step section : step-section #107
step list : step-list #108

yolophg · 2024-07-08T19:10:37Z

사소한 추가 코멘트가 있지만 전반적으로 완성이 된 것 같아서 승인합니다. @sounmind 님께서, #76 (comment) 에서 피드백 드렸던 것처럼, PR의 커밋 이력을 좀 더 깔끔하게 관리하시면 팀원 간에 코드 리뷰 경험을 개선하는데 큰 도움이 될 거라 생각합니다.

@sounmind @DaleSeo

넵, 코드 리뷰 경험을 개선하는데에 커밋이력을 깔끔히 관리하면 좋은 점 동의합니다. merge 커밋들의 경우, 제가 conflict를 해결하려고 했었는데 이 부분은 혼선을 드린 것 같아 다음 작업부터는 제외시키도록 하겠습니다!
근데, 커밋 이력을 '깔끔히' 한다는 것에 대한 기준이 저희끼리도 어느정도 논의가 되면 좋을 것 같아 코멘트 남겨봅니다.

저의 경우에는, 평소에 작업할 때 최대한 작은 단위로 쪼개서 그 커밋에는 그 내용(사소한 리네이밍과 같은 리팩토링도)만 포함되는걸 선호해왔고, 이번 컴포넌트에서도 아주 작은 단위부터 쪼개다보니 merge 커밋을 제외하고도 내용이 꽤 되었던 것 같습니다.
사실, 이 기준이 다들 다르고 애매한 것 같긴 한데, 이와 관련해서도 논의해보면 좋을 것 같습니다!

ex)
생각나는 예시를 들어보면, pr 리뷰를 반영해야하는 상황이라고 할 때, 리네이밍, 불필요한 코드 제거 이렇게 두가지를 반영해야한다고 가정해보면,

원래의 저라면) rename, remove unnecessary function으로 나누어 커밋을 했을 것 같습니다.
이것을 같이 합쳐서 정리하면) reflected pr reviews 와 같이 정리해서 커밋하는 방법도 있을 것 같습니다.

따라서, 저희의 논의로 명확하게 방식을 정할 수 있는 영역인지는 모르겠으나, 최대한 제가 묶을 수 있는 것들끼리는 묶어서 커밋할 수 있도록 신경을 써보겠습니다 :)

DaleSeo · 2024-07-08T20:03:56Z

근데, 커밋 이력을 '깔끔히' 한다는 것에 대한 기준이 저희끼리도 어느정도 논의가 되면 좋을 것 같아 코멘트 남겨봅니다.

@yolophg 네, 저는 다 같이 논의하는 거는 좋습니다. 그런데 한 가지 생각해볼 부분은 이러한 세세한 컨벤션에 대한 논의가 때때로 오히려 프로젝트의 진행을 느리게 할 수 있다는 것입니다. 이미 예상하시고 계신 것처럼 깔끔한 커밋 이력에 대한 판단은 상당히 주관적일 수 있는 부분이라서, 저도 여러 프로젝트에서 관련된 논의를 해봤지만 한 번도 모든 팀원이 만족할 만한 합의를 이끌어 낸 적이 거의 없었습니다. 오히려 억지 같은 규칙을 만들고 실천이 안 됐던 기억이 있습니다.

@sounmind 님만 불편하지 않으시다면 저는 @yolophg 님의 잘게 쪼개시는 커밋 취향을 존중하고 싶습니다. 지금 이대로 머지하셔도 크게 개의치는 않고요. 단, 말씀하셨던 것처럼 merge 커밋만 정리해주셔도 나중에 기억을 되살리기 위해서 이 PR을 참조할 때 큰 도움이 될 것 같습니다.

모임 때 보면 항상 시간이 부족했던 것 같아서, 꼭 중요한 논의에 시간을 쓰고 싶어서 제 생각을 남겨봅니다.

sounmind · 2024-07-08T20:06:57Z

@yolophg 의견 주셔서 감사합니다!
@DaleSeo 동의합니다. 자동화할 수 없는 규칙이라면, 자율에 맡기는 게 좋을 것 같습니다 ☺️

…to BaseElement

style: format StepTextLink HTML for better readability fix: restore accidentally removed text in ds-review-item style: Remove unused style code from index.html style: add large media query in Step component

yolophg · 2024-07-08T22:08:58Z

@sounmind @DaleSeo
머지 커밋 이력+비슷한 류의 커밋들은 묶어서 정리하여 머지합니다!
의견 모두 감사드립니다 :)

yolophg requested a review from a team as a code owner June 25, 2024 20:21

yolophg marked this pull request as draft June 25, 2024 20:21

github-actions bot assigned yolophg Jun 25, 2024

yolophg linked an issue Jul 3, 2024 that may be closed by this pull request

Create Step Component #75

Closed

yolophg marked this pull request as ready for review July 3, 2024 18:40

sounmind reviewed Jul 3, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

sounmind reviewed Jul 3, 2024

View reviewed changes

components/step.js Outdated Show resolved Hide resolved

sounmind reviewed Jul 3, 2024

View reviewed changes

DaleSeo reviewed Jul 4, 2024

View reviewed changes

yolophg force-pushed the helena/75-create-step-component branch 5 times, most recently from 3df61d5 to 1afb4c5 Compare July 5, 2024 09:06

DaleSeo approved these changes Jul 6, 2024

View reviewed changes

components/step-section.js Outdated Show resolved Hide resolved

components/step-section.js Outdated Show resolved Hide resolved

components/step-section.js Show resolved Hide resolved

DaleSeo mentioned this pull request Jul 8, 2024

Meeting 7 #111

Closed

8 tasks

yolophg force-pushed the helena/75-create-step-component branch 2 times, most recently from 7786000 to 155639d Compare July 8, 2024 21:54

yolophg added 7 commits July 8, 2024 18:01

feat: add step component

754aa98

feature: add step text link component and re-design step component

e211d89

feat: re-design step component

636695f

style: modify some of styles

78cd940

style: add color

b13a661

feat: add step component responsive

f843c2e

refactor: Remove duplicate code by extracting common functionality in…

bcff54a

…to BaseElement

yolophg added 3 commits July 8, 2024 18:03

refactor: rename BaseElement to BaseStepElement

2d2af01

style: format StepTextLink HTML for better readability fix: restore accidentally removed text in ds-review-item style: Remove unused style code from index.html style: add large media query in Step component

feat: add stepList and stepSection

933e7bc

refactor: reflected pr reviews

789461e

yolophg force-pushed the helena/75-create-step-component branch from 155639d to 789461e Compare July 8, 2024 22:06

This was linked to issues Jul 8, 2024

review-list #109

Closed

code-review section #110

Closed

yolophg merged commit 3fe2cb3 into main Jul 8, 2024

yolophg deleted the helena/75-create-step-component branch July 8, 2024 22:10

yolophg removed a link to an issue Jul 8, 2024

review-list #109

Closed

yolophg linked an issue Jul 8, 2024 that may be closed by this pull request

step-list #108

Closed

yolophg removed a link to an issue Jul 8, 2024

code-review section #110

Closed

yolophg linked an issue Jul 8, 2024 that may be closed by this pull request

step-section #107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add step component #76

feat: add step component #76

yolophg commented Jun 25, 2024 •

edited

Loading

sounmind Jul 3, 2024

sounmind Jul 3, 2024

sounmind Jul 3, 2024

yolophg Jul 3, 2024 •

edited

Loading

DaleSeo Jul 4, 2024

sounmind Jul 4, 2024 •

edited

Loading

sounmind commented Jul 3, 2024 •

edited

Loading

DaleSeo Jul 4, 2024

sounmind Jul 4, 2024

DaleSeo Jul 6, 2024

DaleSeo Jul 4, 2024

DaleSeo left a comment •

edited

Loading

yolophg commented Jul 8, 2024

yolophg commented Jul 8, 2024

DaleSeo commented Jul 8, 2024 •

edited

Loading

sounmind commented Jul 8, 2024

yolophg commented Jul 8, 2024

		@@ -0,0 +1,132 @@
		import { css, html } from "../html-css-utils.js";

		class BaseStepElement extends HTMLElement {

feat: add step component #76

feat: add step component #76

Conversation

yolophg commented Jun 25, 2024 • edited Loading

Checklist before merging

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yolophg Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sounmind Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

sounmind commented Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaleSeo left a comment • edited Loading

Choose a reason for hiding this comment

yolophg commented Jul 8, 2024

yolophg commented Jul 8, 2024

DaleSeo commented Jul 8, 2024 • edited Loading

sounmind commented Jul 8, 2024

yolophg commented Jul 8, 2024

yolophg commented Jun 25, 2024 •

edited

Loading

yolophg Jul 3, 2024 •

edited

Loading

sounmind Jul 4, 2024 •

edited

Loading

sounmind commented Jul 3, 2024 •

edited

Loading

DaleSeo left a comment •

edited

Loading

DaleSeo commented Jul 8, 2024 •

edited

Loading