branch for codereview #36

jinwoo-kim-nhn · 2018-05-31T01:25:57Z

work

Image-editor에 디자인된 기본 UI 적용
- ui.js 추가 - ui 관련 dom관리 및 이벤트 관리.
- ui/{메뉴명}.js - 메뉴별로 dom관리 및 이벤트 관리
- action.js 추가 - 이벤트 핸들러부분을 전부 관리.
- theme.js 추가 - 테마관리
- polyfill.js 추가 - svg use, classList, element closest
- ui/template - 템플릿 html

demo

http:https://10.78.9.21:8082/examples/example03-theme.html

seonim-ryu · 2018-05-31T06:23:33Z

src/js/graphics.js

@@ -284,6 +292,10 @@ class Graphics {
 this._canvas.setActiveObject(target);
 }

+ setCropSelectionStyle(style) {


이건 퍼블릭 메서드인가요? 주석이 빠졌네요.

seonim-ryu · 2018-05-31T06:27:39Z

src/js/imageEditor.js

 import Invoker from './invoker';
+import Ui from './ui';


요건 네이밍을 정리하면 좋을것 같아요~

seonim-ryu · 2018-05-31T06:28:08Z

src/js/imageEditor.js

@@ -26,9 +29,19 @@ const {isUndefined, forEach, CustomEvents} = snippet;
 class ImageEditor {
 constructor(wrapper, option) {
 option = snippet.extend({
+ includeUi: false,


UI 대문자로 쓰는게 맞을듯요

seonim-ryu · 2018-05-31T06:32:55Z

src/js/ui/draw.js

+ drawRange: new Range(selector('#draw-range'), {
+ min: 5,
+ max: 30,
+ value: 12


상수 모듈 하나 만들어서 거기서 관리하는건 어떨까요?

seonim-ryu · 2018-05-31T06:33:34Z

src/js/ui/draw.js

+ /**
+ * Add event for draw
+ * @param {Object} actions - actions for crop
+ * @param {Function} setDrawMode - set draw mode


두 번째 파라미터는 잘못 들어간 건가요?

seonim-ryu · 2018-05-31T06:34:55Z

src/js/ui/filter.js

+ const option = {};
+ switch (type) {
+ case 'removeWhite':
+ option.threshold = parseInt(this._el.removewhiteThresholdRange.getValue(), 10);


parseInt() 이건 유틸 메서드로 빼서 처리하는게 좋겠어요

seonim-ryu · 2018-05-31T06:37:47Z

src/js/ui/filter.js

+ * @private
+ */
+ _makeSelectOptionList(selectlist) {
+ const blendOptions = ['add', 'diff', 'subtract', 'multiply', 'screen', 'lighten', 'darken'];


메서드 호출할 때마다 이 변수들 매번 생성할 필요는 없어보여요.

seonim-ryu · 2018-05-31T06:38:34Z

src/js/ui/filter.js

+ * @returns {string}
+ * @private
+ */
+ _toCamelCase(targetString) {


이런 로직도 필터 클래스가 가질 필요가 없네요. 유틸로 빼주세요~

seonim-ryu · 2018-05-31T06:45:12Z

src/js/ui/icon.js

+
+ if (file) {
+ imgUrl = URL.createObjectURL(file);
+ registCustomIcon(imgUrl, file);


파일 객체 유무 판별하는 로직도 유틸로 빼주세요~

seonim-ryu · 2018-05-31T06:55:16Z

src/js/ui/icon.js

+ 'icon-location': 'M24 62C8 45.503 0 32.837 0 24 0 10.745 10.745 0 24 0s24 10.745 24 24c0 8.837-8 21.503-24 38zm0-28c5.523 0 10-4.477 10-10s-4.477-10-10-10-10 4.477-10 10 4.477 10 10 10z',
+ 'icon-heart': 'M49.994999,91.349998 l-6.96,-6.333 C18.324001,62.606995 2.01,47.829002 2.01,29.690998 C2.01,14.912998 13.619999,3.299999 28.401001,3.299999 c8.349,0 16.362,5.859 21.594,12 c5.229,-6.141 13.242001,-12 21.591,-12 c14.778,0 26.390999,11.61 26.390999,26.390999 c0,18.138 -16.314001,32.916 -41.025002,55.374001 l-6.96,6.285 z ',
+ 'icon-bubble': 'M44 48L34 58V48H12C5.373 48 0 42.627 0 36V12C0 5.373 5.373 0 12 0h40c6.627 0 12 5.373 12 12v24c0 6.627-5.373 12-12 12h-8z'
+ };


이것도 아이콘 패쓰 모듈로 해서 상수로 빼는게 좋겠어요.

seonim-ryu · 2018-05-31T06:56:26Z

src/js/ui/icon.js

+ iconSubMenu.className = 'icon';
+ iconSubMenu.innerHTML = iconHtml({iconStyle});
+
+ subMenuElement.appendChild(iconSubMenu);


이 메서드는.. 클래스마다 다 가지고 있는 것 같은데..
인터페이스 하나 두고 재사용할 수는 없는건가요?

seonim-ryu · 2018-05-31T06:58:25Z

src/js/ui/shape.js

+ const [shapeType] = button.className.match(/(circle|triangle|rect)/);
+
+ event.currentTarget.classList.remove(this.type);
+ event.currentTarget.classList.add(shapeType);


이 코드도 다른 클래스랑 많이 중복되네요~

seonim-ryu · 2018-05-31T07:21:38Z

src/js/ui/shape.js

+ subMenuElement.appendChild(shapeSubMenu);
+ }
+}
+


[5/31] 오늘은 여기까지 리뷰할게요~ 내일 이어서 진행하겠습니다.

seonim-ryu · 2018-06-01T06:14:23Z

src/js/ui/theme/standard.js

@@ -0,0 +1,95 @@
+/**
+ * @fileoverview The standard theme


파일명 basic은 어떤가요ㅎ

seonim-ryu · 2018-06-01T06:17:30Z

src/js/ui/theme/theme.js

+ linkElement.setAttribute('rel', 'stylesheet');
+ linkElement.setAttribute('type', 'text/css');
+ linkElement.setAttribute('href', `data:text/css;charset=UTF-8,${styleData}`);
+ head.appendChild(linkElement);


이 메서드도 테마 클래스 소유가 아니라 유틸로 빼야겠어요~

seonim-ryu · 2018-06-01T06:27:19Z

src/js/ui/tools/range.js

+ };
+
+ document.addEventListener('mousemove', changeAngle);
+ document.addEventListener('mouseup', stopChangingAngle);


핸들러 함수들 외부 메서드로 분리하는게 가독성에 더 좋을것 같아요

seonim-ryu · 2018-06-01T06:28:36Z

test/ui.spec.js

@@ -0,0 +1,351 @@
+/**
+ * @author NHN Ent. FE Development Team <[email protected]>
+ * @fileoverview Test cases of "src/js/component/cropper.js"


이것은.. 설명이 잘못된 것 같습니다ㅎㅎ

seonim-ryu · 2018-06-01T06:53:27Z

src/js/ui/theme/standard.js

+ menu: {
+ icon: {
+ normal: {
+ path: '../dist/svg',


패쓰 이렇게 잡혀도 상관 없는건가요??

seonim-ryu · 2018-06-01T07:00:04Z

[6/1] 테스트가 깨지는군요 ㅜㅜ 차주에 리팩토링 하시고 리뷰 한 번 더 가시죠~

shiren

수고하셨어요.. 테스트는 꼭해야할 부분들이 몇가지 더 있는것 같아요.

shiren · 2018-06-01T05:32:46Z

src/js/extension/cropzone.js

+ target[1] = outer[1] + size;
+ target[2] = outer[1] + (size * 2);
+ target[3] = outer[2];
+ };


outer하고 size 를 받어서 inner를 리턴해주는 함수로 바꿔서 모듈 안에 private하게 두고 사용하는게 좋을것같아요.

shiren · 2018-06-01T05:43:34Z

src/js/imageEditor.js

+ * Ui instance
+ * @type {Ui}
+ */
+ this.ui = option.includeUi ? new Ui(wrapper, option.includeUi, this.getActions()) : null;


includeUi가 true인경우 ui를 사용하는 조건은 3항연산자가 어울리지 않는것 같아요. null이 따로 의미가 없다면 말이죠
A or B로 선택하는게 아니니까

if (options.includeUi) { this.ui = new Ui(wrapper, option.includeUi, this.getActions()) } 이게 더 의미적으로 맞는것 같습니다. this.ui가 undefined 면 없는것이니까요

shiren · 2018-06-01T05:54:39Z

src/js/ui/draw.js

+ this._el.lineSelectButton.classList.add(lineType);
+
+ this.type = lineType;
+ this.setDrawMode();


click 하면 무었을 한다라는 함수명으로 하나 뽑아주면 click되면 무엇을 하는구나 하고 한눈에 읽힐 수 있을것 같아요. 다시말해 click은 일반화된 행위이고 그 행위로 실제 어떤 작업을 하게되는지를 나타내는 함수명

여기서는 아래 와 같으면 되려나.
this._el.lineSelectButton.addEventListener('click', this.changeDrawModeType.bind(this));

아래 리스너들도 마찮가지

shiren · 2018-06-01T06:29:27Z

src/js/ui/draw.js

+ const selector = str => subMenuElement.querySelector(str);
+ this._makeSubMenuElement(subMenuElement, iconStyle);
+
+ this._el = {


여긴 this._els 가 나을것 같아요 this._el 로 보통 한개의 dom 노드를 참조하는 케이스가 많다보니 햇갈려요.
그리고 일괄처리가 필요해서 모와둔게 아니고 단순히 모와두기만 한것이라면 굳이 객체로 모와서 사용하지 말고
개별적으로 사용해도 괜찮다고 생각합니다.

this._lineSelectButtonEl

이런식으로..

shiren · 2018-06-01T06:37:06Z

src/js/ui/filter.js

+ */
+ _makeSelectOptionList(selectlist) {
+ const blendOptions = ['add', 'diff', 'subtract', 'multiply', 'screen', 'lighten', 'darken'];
+ snippet.forEach(blendOptions, option => {


인자가 두개 이상부터는 괄호로 감싸는것이 컨벤션으로 아는데 린터가 안잡았나?

shiren · 2018-06-01T07:02:19Z

src/js/ui/tools/colorpicker.js

+ */
+ setColor(color) {
+ this._color = color;
+ this._changeColorElement(color);


여기도 게터 세터 딱이네.

shiren · 2018-06-01T07:03:12Z

src/js/ui/tools/colorpicker.js

+ _setPickerControlPosition() {
+ const controlStyle = this.pickerControl.style;
+ const top = parseInt(window.getComputedStyle(this.pickerControl, null).height, 10) + 12;
+ const left = (parseInt(window.getComputedStyle(this.pickerControl, null).width, 10) / 2) - 20;


매직넘버 제거해주세요

shiren · 2018-06-01T07:03:28Z

src/js/ui/tools/range.js

+ this.value = options.value || 0;
+ this.rangeElement = rangeElement;
+ this._drawRangeElement();
+ this.rangeWidth = parseInt(window.getComputedStyle(rangeElement, null).width, 10) - 12;


매직넘버

shiren · 2018-06-01T07:04:09Z

src/js/ui/tools/range.js

+ * @param {Number} value range value
+ * @param {Boolean} fire whether fire custom event or not
+ */
+ setValue(value, fire = true) {


여기도 게터 세터

shiren · 2018-06-01T07:05:47Z

src/js/util.js

+ * @returns {string} rgb expression
+ */
+ getRgb(color, alpha) {
+ const r = parseInt(color.slice(1, 3), 16);


컬러값이 '#fff'

이렇게 축약형일때도 고려되야 하는거 아니에요?

seonim-ryu · 2018-06-04T06:32:03Z

src/js/component/shape.js

@@ -308,6 +308,11 @@ class Shape extends Component {
 * @private
 */
 _onFabricMouseDown(fEvent) {
+ if (!fEvent.target) {
+ this._isSelected = false;
+ this._shapeObj = false;


이건 false 말고 null이 들어가는게 맞을 것 같아요.

seonim-ryu · 2018-06-04T06:34:13Z

src/js/consts.js

+ 'icon-polygon': 'M3,31 L19,3 h32 l16,28 l-16,28 H19 z ',
+ 'icon-location': 'M24 62C8 45.503 0 32.837 0 24 0 10.745 10.745 0 24 0s24 10.745 24 24c0 8.837-8 21.503-24 38zm0-28c5.523 0 10-4.477 10-10s-4.477-10-10-10-10 4.477-10 10 4.477 10 10 10z',
+ 'icon-heart': 'M49.994999,91.349998 l-6.96,-6.333 C18.324001,62.606995 2.01,47.829002 2.01,29.690998 C2.01,14.912998 13.619999,3.299999 28.401001,3.299999 c8.349,0 16.362,5.859 21.594,12 c5.229,-6.141 13.242001,-12 21.591,-12 c14.778,0 26.390999,11.61 26.390999,26.390999 c0,18.138 -16.314001,32.916 -41.025002,55.374001 l-6.96,6.285 z ',
+ 'icon-bubble': 'M44 48L34 58V48H12C5.373 48 0 42.627 0 36V12C0 5.373 5.373 0 12 0h40c6.627 0 12 5.373 12 12v24c0 6.627-5.373 12-12 12h-8z'


프로퍼티들 대문자로 사용해야 하지 않을까요?

svg같은데 꼭 자바스크립트에 있어야 하나요? css에 있는 것이 어울리는데.

seonim-ryu · 2018-06-04T06:36:55Z

src/js/imageEditor.js

@@ -26,9 +29,21 @@ const {isUndefined, forEach, CustomEvents} = snippet;
 class ImageEditor {
 constructor(wrapper, option) {


이번에 정리하는김에.. option -> options로 변경하면 좋겠습니다~

seonim-ryu · 2018-06-04T06:37:43Z

src/js/imageEditor.js

@@ -848,6 +875,15 @@ class ImageEditor {
 return this.execute(commands.CHANGE_TEXT_STYLE, id, styleObj);
 }



프라이빗 주석 추가요

seonim-ryu · 2018-06-04T06:38:50Z

src/js/ui/draw.js

+ /**
+ * Change drawing color
+ * @param {string} color - select drawing color
+ */


@private 추가요~

seonim-ryu · 2018-06-04T06:39:13Z

src/js/ui/draw.js

+ /**
+ * Change drawing Range
+ * @param {number} value - select drawing range
+ */


여기도 @private 추가요.

kyuwoo-choi · 2018-06-04T06:57:43Z

src/js/ui/draw.js

+ */
+ _changeDrawColor(color) {
+ color = color || 'transparent';
+ this.color = color;


한줄로 해도 될 것 같아요.

kyuwoo-choi · 2018-06-04T07:00:42Z

src/js/ui/filter.js

+ rangeWrap.appendChild(rangelabel);
+ rangeWrap.appendChild(range);
+ pickerControl.appendChild(rangeWrap);
+ pickerControl.style.height = '130px';


이것도 css로 빠지기 힘든 건가요?

kyuwoo-choi · 2018-06-04T07:01:30Z

src/js/ui/filter.js

+ return new Range(range, {
+ min: 0,
+ max: 1,
+ value: 0.7


이런것들도 상수 모아놓는 파일이 있었던것 같은데, 같이 관리하면 좋겠어요.

kyuwoo-choi · 2018-06-04T07:02:26Z

src/js/ui/filter.js

+ this._makeSelectOptionList(selectlist);
+
+ pickerControl.appendChild(selectlistWrap);
+ pickerControl.style.height = '130px';


위에랑 같은거네요. 불가피 하다면, 한번에 바꿀 수 있도록 여기서 상수로라도 처리해야 겠습니다.

kyuwoo-choi · 2018-06-04T07:08:48Z

src/js/ui/flip.js

+export default class Flip extends Submenu {
+ constructor(subMenuElement, {iconStyle}) {
+ super(subMenuElement, {
+ name: 'flip',


UI마다 이름 처리를 해주어야 하는군요.

babel class properties 사용중이라면 프로퍼티로 넣고,
UI 상위 constructor에서 이름 처리해주는 것도 깔끔하겠네요.

kyuwoo-choi · 2018-06-04T07:10:48Z

src/js/ui/rotate.js

+ const rotateType = this.getButtonType(button, ['counterclockwise', 'clockwise']);
+ const rotateAngle = {
+ 'clockwise': 30,
+ 'counterclockwise': -30


이부분도 상수처리 필요하겟습니다.

kyuwoo-choi · 2018-06-04T07:11:20Z

src/js/ui/shape.js

+ });
+ this.type = 'rect';
+ this.options = {
+ stroke: '#ffbb3b',


상수처리는 이 이후로 생략하겠습니다.

kyuwoo-choi · 2018-06-04T07:12:31Z

src/js/ui/text.js

+ * Get text color
+ * @returns {string} - text color
+ */
+ get textColor() {


우리 엑세서 안쓰기로 했던 것 같아요.

kyuwoo-choi · 2018-06-04T07:13:36Z

[6/4] 완료입니다. 많네요;;; 수고하셨어요~

seonim-ryu · 2018-06-04T07:16:31Z

src/js/ui/icon.js

+ /**
+ * Change icon color
+ * @param {string} color - color for change
+ */


전체적으로 클래스마다 @private이 누락된것 같네요

seonim-ryu · 2018-06-04T07:17:40Z

src/js/ui/mask.js

+ * @param {object} event - File change event object
+ */
+ _loadMaskFile(event) {
+ const supportingFileAPI = !!(window.File && window.FileList && window.FileReader);


이건 유틸 메서드로 안빼고 그대로 사용하는건가요? ;ㅅ;

seonim-ryu · 2018-06-04T07:18:32Z

src/js/ui/rotate.js

+ const rotateType = this.getButtonType(button, ['counterclockwise', 'clockwise']);
+ const rotateAngle = {
+ 'clockwise': 30,
+ 'counterclockwise': -30


하이픈으로 연결된 경우 아니면 프로퍼티들 따옴표 없이 사용해도 되요~

seonim-ryu · 2018-06-04T07:20:36Z

src/js/ui/submenuBase.js

+ * Get butten type
+ * @param {HTMLElement} button - event target element
+ * @param {array} buttonNames - Array of button names
+ * @returns {string} - button type


음? 리턴값 배열 아닌가요?

seonim-ryu · 2018-06-04T07:20:53Z

src/js/ui/submenuBase.js

+ * @returns {string} - button type
+ */
+ getButtonType(button, buttonNames) {
+ const [buttonType] = button.className.match(RegExp(`(${buttonNames.join('|')})`));


할당하지 말고 바로 리턴해도 되겠어요~

seonim-ryu · 2018-06-04T07:24:11Z

src/js/ui/tools/colorpicker.js

+ colorpickerElement.addEventListener('click', event => {
+ this._show = !this._show;
+ const display = this._show ? 'block' : 'none';
+ this.pickerControl.style.display = display;


함수 할당 안하고 바로 써도 되겠어요~

seonim-ryu · 2018-06-04T07:26:21Z

[6/4] 리뷰 완료하였습니다. TC가 깨졌네요 ㅜㅜ 고생하셨어요~

jinwoo-kim-nhn added 2 commits May 31, 2018 10:13

branch for codereview

572f7f0

testcode for codereview

1c63089

seonim-ryu reviewed May 31, 2018

View reviewed changes

seonim-ryu reviewed Jun 1, 2018

View reviewed changes

src/js/ui/theme/standard.js

@@ -0,0 +1,95 @@

/**

* @fileoverview The standard theme

Copy link

Member

seonim-ryu Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파일명 basic은 어떤가요ㅎ

seonim-ryu reviewed Jun 1, 2018

View reviewed changes

shiren reviewed Jun 1, 2018

View reviewed changes

apply code review

3df44e3

seonim-ryu reviewed Jun 4, 2018

View reviewed changes

src/js/ui/draw.js

/**

* Change drawing color

* @param {string} color - select drawing color

*/

Copy link

Member

seonim-ryu Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@private 추가요~

seonim-ryu reviewed Jun 4, 2018

View reviewed changes

src/js/ui/draw.js

/**

* Change drawing Range

* @param {number} value - select drawing range

*/

Copy link

Member

seonim-ryu Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 @private 추가요.

kyuwoo-choi reviewed Jun 4, 2018

View reviewed changes

seonim-ryu reviewed Jun 4, 2018

View reviewed changes

jinwoo-kim-nhn closed this Jun 18, 2018

jinwoo-kim-nhn deleted the feat/design-codereview branch July 9, 2018 08:21

		@@ -26,9 +29,21 @@ const {isUndefined, forEach, CustomEvents} = snippet;
		class ImageEditor {
		constructor(wrapper, option) {

		@@ -848,6 +875,15 @@ class ImageEditor {
		return this.execute(commands.CHANGE_TEXT_STYLE, id, styleObj);
		}

branch for codereview #36

branch for codereview #36

Conversation

jinwoo-kim-nhn commented May 31, 2018 • edited

work

demo

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonim-ryu commented Jun 1, 2018

shiren left a comment

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyuwoo-choi commented Jun 4, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonim-ryu commented Jun 4, 2018

jinwoo-kim-nhn commented May 31, 2018 •

edited