-
Notifications
You must be signed in to change notification settings - Fork 1
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
[#5] 폴더구조 수정 및 라우팅 설정 #6
Conversation
import { router } from './core/router/Router.js'; | ||
|
||
document.addEventListener('DOMContentLoaded', () => { | ||
router.init(); |
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.
index 의 역할은 라우터만 init 해주는 건가요?
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.
네 맞습니다 index.js는 시작점으로 라우팅 관련된 코드는 라우팅 파일로 옮겨 응집도를 높혔습니다
} | ||
|
||
render() { | ||
const headerTag = this.getAttribute('header') || ''; |
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.
오호...? 이 코드는 엄청 많이 생길거 같은데... 너무 리액트스럽게 쓰고자 고민하고 계신거 같기도 하네요.
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.
레이아웃에 관련된 리뷰와 같이 과한 설계를 한 느낌이 약간 듭니다.. 유연한 레이아웃 변경을 위해 처리했는데 오히려 관리할 리소스가 늘은 느낌이 드는 것 같습니다
|
||
render() { | ||
const headerTag = this.getAttribute('header') || ''; | ||
const footerTag = this.getAttribute('footer') || ''; |
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.
헤더와 푸터를 이렇게 다이나믹하게 주입해주는 장점이 뭘까여? 레이아웃안에 헤더와 푸터를 선언해놓은 것과 대비한 장점이 궁금해요. 헤더와 푸터가 자주 바뀌는 대상일까요?
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.
헤더와 푸터를 다이나믹하게 처리함으로써 유연한 레이아웃 관리가 가능하여 레이아웃이 다른 페이지가 나올 경우 쉽게 변경이 가능합니다.
해더,푸터는 자주 바뀌는 대상은 아니지만 바뀔 수도 있는 대상입니다. 그렇기 때문에 변경 가능성을 열어두었습니다.
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.
헤더나 푸터를 주입받도록 한다고 해도 결국에는 custom tag 를 전달하는 식일테고 이 변경이 라우터마다 config 로 추가되는게 유연한 구조인지 의문이 드네요.
프로젝트 완성되어 갈즈음 얼만큼 활용되었는지 봐도 좋을거 같네요. 지금으로서는 섣부른 확장고려 인거 같긴해요.
애초에 페이지가 하나짜리라고도 하셨기에..?
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.
네 맞습니다..페이지가 하나인데... 저번 멘토링에 레이아웃 구조가 확장에 유연하지 않다라고 말씀 하신게 생각나서 이렇게 한번 해볼까?라는 생각으로 수정을 해 본 케이스입니다
}); | ||
|
||
document.addEventListener('click', (e) => { | ||
const link = e.target.closest('[data-link]'); |
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.
a tag 를 쓰지않고 커스텀 dataset을 이용하는 장점과 단점은 뭘까요?
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.
dataset을 사용하면 closest을 통해 클릭 된 요소로부터 속성을 가진 가장 가까운 상위 요소를 찾게 되면서 원하는 라우팅 링크만 정확하게 선택 할 수 있다는 장점이 있습니다.
단점으로는 closest은 이벤트가 발생시 상위 요소로 올라가며 DOM을 탐색하기에 구조가 깊어질 수록 탐색 리소스가 많이 발생한다는 단점이 있고 SEO와 접근성 측면에서도 불리하다는 단점이 있습니다.
후자의 단점은 a 태그와 dataset을 함께 사용하면 해결 �됩니다.
<a href="/home" data-link>홈</a>
@@ -0,0 +1,49 @@ | |||
export default class PropertyInput extends HTMLElement { |
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.
재사용성을 많이 고민하신 듯하네요. 👍
import '../components/CanvasLayerSidebar.js'; | ||
import '../components/CanvasToolsSidebar.js'; | ||
|
||
export default class CanvasPage extends HTMLElement { |
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.
이 페이지가 feature 안에 존재하는게 조금 어색해요.
page 는 일반적으로 루트에 존재하는게 위계상 적절할거 같아요.
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.
src/
├── pages/
│ └── canvas/
│ └── CanvasPage.js
├── features/
│ └── canvas/
│ ├── components/
...
제가 이해한 바가 위와 같은데 맞을까요??
관련 기능을 모아놓고 응집도를 높이려고 하다보니까 이렇게 된 것 같습니다.
저도 보면서 어색한데...라는데 생각을 했는데 이렇게 수정하면 구조적으로 더 직관성이 높을 것 같습니다
src/core/router/RouterConfig.js
Outdated
export const routes = { | ||
'*': { | ||
component: CanvasPage, | ||
layout: { |
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.
이걸 굳이 따로 config 에 선언해야 하나 싶네요.
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.
처음에 Route파일에 작성했지만 관심사를 분리하고 싶어서 분리했습니다
라우팅 설정과 라우터 로직을 분리하는 목적으로 따로 설정했습니다
Quality Gate passedIssues Measures |
Related Issue : #5
TODO
COMMENT
�기능 기반 폴더 구조
기존 명확한 폴더 구조가 없어 기능 기반으로 폴더구조를 잡았습니다.
해당 구조로 잡은 이유는 기능 단위로 높은 응집도로 모여 있어 관리가 쉬울 것으로 생각되고 기능 별로 독립적인 폴더를 가지고 있어 추후 기능 추가 시 기존 코드에 영향이 적을 것으로 생각되어 선택했습니다.
라우팅 컴포넌트
태그를 사용하여 라우팅 콘텐츠를 노출 했으나 의미적으로 좀 더 명확 할 필요가 있다고 생각하여 라우팅 처리하는 컴포넌트를 만들어 수정했습니다.기존
해당 기능은 anuglar의 에서 영감을 얻어 적용했습니다.
레이아웃 유연성 고려한 설계
지난주 멘토링 시 레이아웃의 확장성에 대한 피드백을 받아 수정했습니다.
라우팅 관련 파일을 명확히 분리하였고 레이아웃 설정을 통해 헤더, 푸터를 선택 할 수 있도록 수정했습니다.