-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add review item component #37
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
.review-item { | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
border-radius: 10px; | ||
padding: 41px 51px 31px; | ||
background-color: #efefef; | ||
max-width: 568px; | ||
} | ||
.review-img-text-container { | ||
display: flex; | ||
align-items: center; | ||
} | ||
.review-img { | ||
width: 104px; | ||
height: 104px; | ||
flex-shrink: 0; | ||
margin-right: 31px; | ||
} | ||
.review-img img { | ||
width: 100%; | ||
height: 100%; | ||
padding-top: 7px; | ||
border-radius: 50%; | ||
background-color: #ffffff; | ||
} | ||
.review-text { | ||
font-size: 14px; | ||
} | ||
.review-name { | ||
width: 100%; | ||
margin-top: 14px; | ||
text-align: right; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
class ReviewItem extends HTMLElement { | ||
constructor() { | ||
super(); | ||
this.attachShadow({ mode: "open" }); | ||
} | ||
|
||
// initial render in here to ensure the element is fully connected to the DOM. | ||
connectedCallback() { | ||
this.render(); | ||
} | ||
|
||
static get observedAttributes() { | ||
return ["text", "name", "img"]; | ||
} | ||
|
||
attributeChangedCallback(attrName, oldVal, newVal) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 혹시 constructor 나 connectedCallback이 아닌 observedAttributes과 attributeChangedCallback을 사용에 render method를 넣었을 때 얻을 수 있는 이점을 알 수 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 질문이 혹시, 동적 변경 관련 메소드를 사용한 것에 대한 질문일까요? 아니면, constructor/connectedCallback 말고, attributeChangedCallback에서 render()를 호출했을 때의 차이에 대해 질문하신걸까요? 아니면 둘 다 인걸까요?😄 전자의 대해 답변 드리자면, 첫 의도는 일단, 추후 확장성이나 유연성을 혹시라도 고려하는게 좋지 않을까라는 생각과, 사실 동적인게 더 익숙하고 편안해서 일단은 제가 그렇게 만들고 싶어서 작성을 해본 것도 있는 것 같습니다. 😀 후자의 경우에 대해 답변 드리자면, constructor/connectedCallback에서 호출한 render(저의 경우에는 connectedCallback에서 호출했습니다)와 attributedChangedCallback에서 호출한 render의 의미는 완전히 다르게 의도해서 작성했습니다!
하지만, 결론적으로 동적 변경 기능을 하는 해당 메소드를 제거할 예정이라, render()함수를 포함하고 있던 불필요했던 메소드 또한 사라질테니, 최종적으로 유지보수에 더 좋은 방향일거라고 판단됩니다. 질문해주셔서 감사합니다 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 상세한 답변 감사드립니다! 지금 제 질문을 다시 보니 충분히 구체적으로 질문드리지 않아 오해의 소지가 있고 애매모호한 점이 많네요...ㅎㅎ 우선 질문은 전자에 대한 것 하나입니다! 저도 비슷한 생각으로 확장성을 고려해 동적 변경을 고려하여 개발했다가 전에 달레님께서 정적 유지보수로만 진행할 거라고 짚어주신 기억이 있고 테크리드님의 피드백도 있어서 관련 메서드를 뺐습니다! 혹시나 Helena님께서도 비숫한 생각으로 개발하신 것 같아 여쭤봤습니다! 부족한 질문이었음에도 불구하고 상세히 설명해주셔서 감사드립니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋습니다, 어제 미팅에서도 각자 스타일대로 컴포넌트를 작성 중이었던 점 잠깐 언급됐었는데 요부분을 어느정도는 통일감을 가져가는 것이 유지보수 측면에서도 그렇고 저희가 당장 협업하면서도 혼란이 적을 것 같습니다. |
||
if (oldVal !== newVal) { | ||
this.render(); | ||
} | ||
} | ||
|
||
// getter to make it clean to access the attribute value and ensure a default empty string if the attribute is missing. | ||
get text() { | ||
return this.getAttribute("text") || ""; | ||
} | ||
|
||
get name() { | ||
return this.getAttribute("name") || ""; | ||
} | ||
|
||
get img() { | ||
return this.getAttribute("img") || ""; | ||
} | ||
|
||
render() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inspired from React? 😆 |
||
try { | ||
this.shadowRoot.innerHTML = ` | ||
<link rel="stylesheet" href="components/review-item/review-item.css"> | ||
<div class="review-item"> | ||
<div class="review-img-text-container"> | ||
<div class="review-img"> | ||
<img src="${this.img}" alt="Person"> | ||
</div> | ||
<div class="review-text">${this.text}</div> | ||
</div> | ||
<div class="review-name">${this.name}</div> | ||
</div> | ||
`; | ||
} catch (error) { | ||
// log any rendering errors. | ||
console.error("Error during render:", error); | ||
} | ||
} | ||
} | ||
|
||
customElements.define("review-item", ReviewItem); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connectedCallback을 사용하면 더 안전하게 render할 수 있겠군요! 참고하겠습니다!