Feat/add defalut icon #347
Conversation
src/js/ui/template/submenu/crop.js
Outdated
| <use xlink:href="${active.path}#${active.name}-ic-shape-rectangle" | ||
| class="active"/> | ||
| </svg> | ||
| ${svgIconMaker(['normal', 'active'], 'shape-rectangle', true)} |
There was a problem hiding this comment.
makeSvgIcon이 더 어울리지 않을까요?
There was a problem hiding this comment.
넵 말해주신 네이밍이 맞는것 같아서 일괄 변경하였씁니다. 감사합니다.
src/js/ui.js
Outdated
| snippet.forEach(HELP_MENUS, menuName => { | ||
| this._addTooltipAttribute(this._buttonElements[menuName], menuName); | ||
| _addHelpMenus() { | ||
| const helpMenuWithPartition = [...HELP_MENUS.slice(0, 3), '', ...HELP_MENUS.slice(3), '']; |
There was a problem hiding this comment.
3은 어떤 의미예요? 특정 메뉴 기준으로 순서대로 처리해야 하면 메뉴 배열을 미리 정의하면 좋을 것 같아요. 메뉴 순서가 변경되거나 추가되거나 삭제되면 에러가 날거 같아서요.
There was a problem hiding this comment.
이부분 메뉴가 변경되도 무리가 없도록 개선 하였습니다. 감사합니다.
src/js/ui.js
Outdated
| if (!menuName) { | ||
| this._makeMenuPartitionElement(); | ||
|
|
||
| return; |
There was a problem hiding this comment.
code-snippet의 forEach로 루프를 종료 시키려면 false를 리턴해야 합니다.
루프를 종료시킬 필요가 없으면 저는 배열.forEach()를 바로 쓰는게 어떨까 합니다.
There was a problem hiding this comment.
루프를 종료시킬 의도는 아니었고.. 생각해보니 중간에 return 이 헷갈리는 것 같아서 else 로 처리하였습니다. 리뷰 감사합니다.
src/js/ui.js
Outdated
| _makeMenuPartitionElement() { | ||
| const partitionElement = document.createElement('li'); | ||
| const partitionInnerElement = document.createElement('div'); | ||
| partitionElement.className = 'tui-image-editor-item'; |
There was a problem hiding this comment.
캘린더 v2와 그리드에서는 클래스 이름 패턴을 함수로 정의합니다. 참고해 주세요.
const CSS_PREFIX = 'tui-calendar-';
function cls(str = '', prefix = ''): string {
if (str.charAt(0) === '.') {
return `.${CSS_PREFIX}${prefix}${str.slice(1)}`;
}
return `${CSS_PREFIX}${prefix}${str}`;
}
const className = cls('item');
There was a problem hiding this comment.
괜찮은데요? TOAST UI 공통으로 이런 함수를 가져다 쓰면 좋겠어요
There was a problem hiding this comment.
넵 이부분 좋네요 ㅎㅎ, 일단 ui.js에 먼저 반영하고 보이는대로 점차 고치는 방향으로 수정하겠습니다.
src/js/ui/template/submenu/crop.js
Outdated
| <use xlink:href="${active.path}#${active.name}-ic-crop" | ||
| class="active"/> | ||
| </svg> | ||
| ${svgIconMaker(['normal', 'active'], 'crop', true)} |
There was a problem hiding this comment.
hover가 있으면 배열 인풋이 거의 동일해서 생략할 수도 있겠네요. 의견입니다.
| 'menu.hoverIcon.path': 'icon-c.svg', | ||
| 'menu.hoverIcon.name': 'icon-c', | ||
| 'menu.normalIcon.color': '#8a8a8a', | ||
| // 'menu.normalIcon.path': '#8a8a8a', |
There was a problem hiding this comment.
넵 이부분 문서 작업 pr에서 수정되어서 여기서는 따로 수정하지 않겠습니다. 감사해요 ㅎㅎ
| case 'submenu.icon': | ||
| result = { | ||
| active: this.styles[`${firstProperty}.activeIcon`], | ||
| normal: this.styles[`${firstProperty}.normalIcon`] |
There was a problem hiding this comment.
서브 메뉴는 hover, disabled가 필요 없는거예요?
There was a problem hiding this comment.
넵 서브메뉴는 현재 필요 없습니다.
src/js/ui/theme/theme.js
Outdated
| * Load defulat svg icon | ||
| * @private | ||
| */ | ||
| _defaultSvgIconLoad() { |
There was a problem hiding this comment.
동사 위치가 앞으로 와야 할거 같아요. _loadDefaultSvgIcon
There was a problem hiding this comment.
넵 반영하였습니다. 감사합니다.
* feat: default menu svg icon - middle commit * feat: default icon normal Check basic operation * feat: prototype complete for default icon improve * feat: complate default icon code * example file update for default icon setting * chore: svg-loader dependency -> devdependency * apply codereview * apply codereview - 2
Please check if the PR fulfills these requirements
fix #xxx[,#xxx], where "xxx" is the issue number)Description
작업내용