feat : create image component #47
Conversation
SamTheKorean
commented
Jun 14, 2024
•
SamTheKorean
commented
- 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)
components/image.js
Outdated
| const width = this.getAttribute("width") || ""; | ||
| const height = this.getAttribute("height") || ""; | ||
|
|
||
| const html = ` |
There was a problem hiding this comment.
이 경우엔 html를 상단에 넣으면 속성 값을 가져올 수 없어서 consturctor 안에서 선언했습니다!
There was a problem hiding this comment.
네 맞습니다! 처음에 우려했던 부분이 나타나더라고요..!
|
피드백 반영 및 합의된 컴포넌트 코딩 방식 반영하고 다시 커밋하였습니다! 리뷰 부탁드리겠습니다! |
components/image.js
Outdated
| const height = this.getAttribute("height") || ""; | ||
|
|
||
| const html = ` | ||
| <img src="${src}" alt="${alt}" width="${width}" height="${height}"> |
There was a problem hiding this comment.
<img width="" height="" />가 <img />와 동일할까요?
There was a problem hiding this comment.
height가 없을 때 img 엘리먼트 기본 값을 넣어줘야하는데 이 로직으로는 ""값이 들어가겠군요.. 피드백 감사합니다!
SamTheKorean
force-pushed
the
sam/33
branch
4 times, most recently
from
06a22ae to
7e8b0b5
Compare
| @@ -1,6 +1,7 @@ | |||
| import "./components/button-link.js"; | |||
There was a problem hiding this comment.
알파벳순에 맞게 위로 올렸습니다.
|
피드백 및 정해진 컴포넌트 스타일 반영하고 커밋했습니다! |
components/image.js
Outdated
| const allowedAttributes = ["src", "alt", "width", "height"]; | ||
| for (const attr of this.attributes) { | ||
| if (!allowedAttributes.includes(attr.name)) { | ||
| throw new Error(`The attribute "${attr.name}" is not allowed.`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
다음에는 특정 attribute만 허용하기 보다, 필요한 attribute를 전달하지 않았을 때 에러를 던지면 어떨까요?
예를 들어 웹 접근성을 위해 aria-label 같은 image의 주요 스타일과 논리와 상관 없는 attribute를 전달하거나 제거하고 싶을 때
현재 구현으로는 매번 image component 구현을 바꿔야 하는 불편함이 있을 수 있습니다.
물론 그만큼 외부에서 주입하는 변화에 컴포넌트가 취약해질 수도 있겠죠?
There was a problem hiding this comment.
감사합니다! 피드백 반영해서 수정해보겠습니다!
components/image.js
Outdated
| let imgHtml = `<img src="${src}" alt="${alt}"`; | ||
|
|
||
| if (this.hasAttribute("width")) { | ||
| const width = this.getAttribute("width"); | ||
| imgHtml += ` width="${width}"`; | ||
| } | ||
|
|
||
| if (this.hasAttribute("height")) { | ||
| const height = this.getAttribute("height"); | ||
| imgHtml += ` height="${height}"`; | ||
| } | ||
|
|
||
| imgHtml += `>`; |
There was a problem hiding this comment.
아, 이거 코드가 너무 읽기 어려운 것 같은데요... 😅
나중에 수정하시는 분이 실수할 가능성이 높아 보입니다.
