feat: disable context menu (fix #1432) #1515

jajugoguma · 2021-11-15T09:05:03Z

Please check if the PR fulfills these requirements

It's submitted to right branch according to our branching model
It's right issue type on title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has description for the breaking change

Description

Added feature to disable context menu
- Disable the context menu by passing null to the contextMenu option.
- If disable the context menu, the browser default context menu appears when right-click.

To-Be

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

adhrinae

리뷰 완료합니다.

adhrinae · 2021-11-16T05:45:40Z

packages/toast-ui.grid/src/store/contextMenu.ts

-  return observable<ContextMenu>({
-    posInfo: null,
-    createMenuGroups: createMenuGroups || createDefaultContextMenu,
+  return isNull(createMenuGroups)


이 부분은 굳이 삼항연산자로 이렇게 긴 로직을 구분해서 넣는 것 보다는 isNull 일 때 early return 하는거로 처리하는게 어떨까요?

기존 타입 유지하면서 내부 프로퍼티 값을 다르게 하는 것으로 변경하면서 긴 로직에 대한 삼항 연산자 제거했습니다.

adhrinae · 2021-11-16T05:56:43Z

packages/toast-ui.grid/src/view/container.tsx

+      isParentExistWithClassNames(ev.target as HTMLElement, ['cell-header', 'cell-dummy']) ||
+      !this.context.store.contextMenu
+    ) {
+      if (target) {


어떤 때에 target 값이 없는지 궁금하네요

곰곰히 생각해보니 target같은 요소가 없을 상황이 없어서 해당 조건문 제거했습니다.

adhrinae · 2021-11-16T06:04:11Z

packages/toast-ui.grid/src/view/contextMenu.tsx

-  pos: pos || (contextMenu.posInfo?.pos ?? null),
-  menuItems: menuItems || contextMenu.flattenTopMenuItems,
+  pos: pos || (contextMenu?.posInfo?.pos ?? null),
+  menuItems: menuItems || contextMenu!.flattenTopMenuItems,


이건 옵셔널 체닝 활용 안해도 괜찮을까요?

기존에는 해당 내부 로직으로 들어올 일이 없어서 옵셔널 체이닝을 사용하지 않았지만, 변경된 코드에서는 그렇지 않기 때문에 옵셔널 체이닝을 사용하는 것으로 변경했습니다.

lja1018

리뷰 완료합니다.

lja1018 · 2021-11-16T06:11:42Z

packages/toast-ui.grid/docs/ko/contextMenu.md

@@ -3,7 +3,7 @@
 TOAST UI Grid는 `v4.18.0` 버전부터 컨텍스트 메뉴 기능을 제공한다. 셀에서 마우스 우측 버튼을 누르면 컨텍스트 메뉴가 나오며, `contextMenu` 옵션으로 원하는 컨텍스트 메뉴를 설정할 수 있다.

 ### 옵션
-`contextMenu` 옵션은 반드시 함수가 되어야 하며, 함수의 반환값은 아래와 같은 2차원 배열이어야 한다.
+`contextMenu` 옵션은 함수가 되어야 하며, 함수의 반환값은 아래와 같은 2차원 배열이어야 한다.


Suggested change

`contextMenu` 옵션은 함수가 되어야 하며, 함수의 반환값은 아래와 같은 2차원 배열이어야 한다.

`contextMenu` 옵션은 함수나 null이 되어야 하며, 함수의 반환값은 아래와 같은 2차원 배열이어야 한다.

는 어떤가요? 아래에 null일 때 동작을 알려주긴 하는데 옵션 자체는 함수나 null이 될 수 있으니까요.

말씀해주신 내용으로 변경했습니다.

js87zz · 2021-11-16T07:29:36Z

packages/toast-ui.grid/docs/ko/contextMenu.md

@@ -51,6 +51,7 @@ const grid = Grid({

 ![image](https://user-images.githubusercontent.com/37766175/123938532-2a3e0800-d9d2-11eb-9ded-ec4562cb026e.png)

+`contextMenu` 옵션이 `null`이면 컨텍스트 메뉴는 비활성화 된다.


이 경우 브라우저 기본 컨텍스트 메뉴가 활성화된다는 점을 알려주고 이미지도 보여주면 좋곘네요ㅎㅎ

브라우저 기본 컨텍스트가 활성화 된다는 것을 명시하고, 관련된 이미지 추가했습니다.

js87zz · 2021-11-16T07:35:07Z

packages/toast-ui.grid/src/store/contextMenu.ts

+    ? createMenuGroups
+    : observable<ContextMenu>({
+        posInfo: null,
+        createMenuGroups: createMenuGroups || createDefaultContextMenu,


조건에 따라서 createMenuGroups 프로퍼티를 null로 설정하는게 더 낫지 않나요? 가져다 쓰는 쪽에서는 타입을 일관되게 유지할 수 있도록 하는게 이득이 될 것 같아서요.

결국 createMenuGroups가 null로 넘어오는 상황이었어서 가져다 쓰는 쪽에서는 타입을 일관되게 유지할 수 있도록 ContextMenu의 타입 정의에서 null을 제거하고 CreateMenuGroups 타입 정의에 null을 추가했습니다.

js87zz · 2021-11-16T07:37:04Z

packages/toast-ui.grid/types/store/index.d.ts

@@ -26,5 +26,5 @@ export interface Store {
  readonly summary: Summary;
  readonly renderState: RenderState;
  readonly filterLayerState: FilterLayerState;
-  readonly contextMenu: ContextMenu;
+  readonly contextMenu: ContextMenu | null;


위에서 언급한 것처럼 ContextMenu로 타입을 통일하고 스토어 안에서 분기처리하는게 사용하는 쪽에서 좀 더 일관성있게 할 수 있지 않을까요?

위에 언급한 것 처럼 ContextMenu의 타입 정의에서 null을 제거하고 CreateMenuGroups 타입 정의에 null을 추가했습니다.

js87zz · 2021-11-16T07:38:31Z

packages/toast-ui.grid/src/view/container.tsx

+        const mouseupEvent = new MouseEvent('mouseup', {
+          bubbles: true,
+          cancelable: true,
+        });
+        target.dispatchEvent(mouseupEvent);


mouseup 발생했을 때 실행되는 dispatch들을 호출하는게 더 가독성에 좋을 것 같습니다.

mouseup이 발생됐을 때 mousemove 등에 할당한 핸들러를 제거해주어야 합니다. 그러나 mouseup 이벤트에 할당된 핸들러인 clearDocumentEvents가 private 이고 이를 public으로 변경하는게 좋을 것 같지 않아서 기존 처럼 mouseup을 발생 시키는 것으로 유지 했습니다.
다만, 가독성 개선을 위해 helper/mouse에 emitMouseup 함수를 추가하고 이를 호출하는 것으로 변경 했습니다.

js87zz

리뷰 완료입니다! 고생하셨어요~

jajugoguma added 5 commits November 15, 2021 10:47

test: add test for disable context menu

bb32686

feat: disable context menu (fix #1432)

cc54af9

docs: update document

7e5aaaa

chore: change to show browser context menu

5c3e017

chore: fix lint error

35b8170

adhrinae suggested changes Nov 16, 2021

View reviewed changes

lja1018 reviewed Nov 16, 2021

View reviewed changes

js87zz reviewed Nov 16, 2021

View reviewed changes

js87zz approved these changes Nov 16, 2021

View reviewed changes

chore: apply code reviews

f182aa5

jajugoguma merged commit 18cd447 into master Nov 18, 2021

jajugoguma deleted the feat/disable-context-menu branch November 18, 2021 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disable context menu (fix #1432) #1515

feat: disable context menu (fix #1432) #1515

jajugoguma commented Nov 15, 2021

adhrinae left a comment

adhrinae Nov 16, 2021

jajugoguma Nov 17, 2021

adhrinae Nov 16, 2021

jajugoguma Nov 17, 2021

adhrinae Nov 16, 2021

jajugoguma Nov 17, 2021

lja1018 left a comment

lja1018 Nov 16, 2021

jajugoguma Nov 17, 2021

js87zz Nov 16, 2021

jajugoguma Nov 17, 2021

js87zz Nov 16, 2021 •

edited

Loading

jajugoguma Nov 17, 2021

js87zz Nov 16, 2021

jajugoguma Nov 17, 2021

js87zz Nov 16, 2021

jajugoguma Nov 17, 2021

js87zz left a comment

	`contextMenu` 옵션은 함수가 되어야 하며, 함수의 반환값은 아래와 같은 2차원 배열이어야 한다.
	`contextMenu` 옵션은 함수나 null이 되어야 하며, 함수의 반환값은 아래와 같은 2차원 배열이어야 한다.

		@@ -51,6 +51,7 @@ const grid = Grid({

		![image](https://user-images.githubusercontent.com/37766175/123938532-2a3e0800-d9d2-11eb-9ded-ec4562cb026e.png)

		`contextMenu` 옵션이 `null`이면 컨텍스트 메뉴는 비활성화 된다.

feat: disable context menu (fix #1432) #1515

feat: disable context menu (fix #1432) #1515

Conversation

jajugoguma commented Nov 15, 2021

Please check if the PR fulfills these requirements

Description

adhrinae 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

lja1018 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

js87zz Nov 16, 2021 • edited Loading

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

js87zz left a comment

Choose a reason for hiding this comment

js87zz Nov 16, 2021 •

edited

Loading