Skip to content
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

fix: context menu goes out of viewport #1682

Merged
merged 5 commits into from May 26, 2022

Conversation

jajugoguma
Copy link
Contributor

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

  • Fixed an issue where the context menu goes out of the viewport.
    • In the case of the vertical test, it was not possible to test because scrolling occurred automatically when the mouse was actioned due to a version problem with cypress.

As-Is

To-Be


Thank you for your contribution to TOAST UI product. πŸŽ‰ 😘 ✨

Copy link
Member

@dotaitch dotaitch left a comment

Choose a reason for hiding this comment

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

리뷰 μ™„λ£Œν•©λ‹ˆλ‹€.

cy.isInViewport(`.${cls('context-menu')}`);
});

it('should change displaying direction of sub menus when there is no spaces to dispaly sub context menu', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should change displaying direction of sub menus when there is no spaces to dispaly sub context menu', () => {
it('should change displaying direction of submenus when there is no space to display sub context menu', () => {

@@ -8,3 +8,4 @@ export const DISABLED_PRIORITY_CELL = 'CELL';
export const DISABLED_PRIORITY_ROW = 'ROW';
export const DISABLED_PRIORITY_COLUMN = 'COLUMN';
export const HORIZONTAL_PADDING_OF_CELL = 10;
export const DEFATULT_SUB_CONTEXT_MENU_TOP = -6;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const DEFATULT_SUB_CONTEXT_MENU_TOP = -6;
export const DEFAULT_SUB_CONTEXT_MENU_TOP = -6;

return { computedTop, computedLeft };
}

private adjustPosition() {
Copy link
Member

Choose a reason for hiding this comment

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

grid의 μ»¨λ²€μ…˜μ€ 잘 λͺ¨λ₯΄κ² μœΌλ‚˜, position을 μ€„μ—¬μ„œ pos라고 많이 μ“°λ˜λ° μ—¬κΈ°μ„œλŠ” Position이라고 λ˜μ–΄ μžˆλ„€μš”. 상관없이 λ‘˜λ‹€ 쓰인닀면 μˆ˜μ •ν•˜μ§€ μ•ŠμœΌμ…”λ„ λ©λ‹ˆλ‹€!

Copy link

Choose a reason for hiding this comment

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

this.props.posλ₯Ό λ³΄λ‹ˆ pos둜 μ€„μ΄λŠ”λ° ν•œν‘œμž…λ‹ˆλ‹€!

Choose a reason for hiding this comment

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

λͺ¨μ–‘μƒˆλ§Œ λ΄μ„œλŠ” getComputedPos λŠ” adjustPos 의 둜컬 ν•¨μˆ˜λ‘œλ§Œ μžˆμ–΄λ„ λ˜κ±°λ‚˜ ꡳ이 λ‚˜λˆŒ ν•„μš”κ°€ μ—†μ–΄λ³΄μ΄λ„€μš”.

Copy link

@lja1018 lja1018 left a comment

Choose a reason for hiding this comment

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

리뷰 μ™„λ£Œν•©λ‹ˆλ‹€.

return { computedTop, computedLeft };
}

private adjustPosition() {
Copy link

Choose a reason for hiding this comment

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

this.props.posλ₯Ό λ³΄λ‹ˆ pos둜 μ€„μ΄λŠ”λ° ν•œν‘œμž…λ‹ˆλ‹€!

Copy link

@adhrinae adhrinae left a comment

Choose a reason for hiding this comment

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

리뷰 μ™„λ£Œν•©λ‹ˆλ‹€.

@@ -7,6 +7,22 @@ before(() => {
cy.visit('/dist');
});

function createGridWithManyData() {

Choose a reason for hiding this comment

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

보톡 DataλΌλŠ” λ‹¨μ–΄μ—λŠ” LargeλΌλŠ” μˆ˜μ‹μ–΄κ°€ 많이 λΆ™μŠ΅λ‹ˆλ‹€.

large dataset, large data volume λ“±λ“±

Comment on lines 137 to 138
const height = 600;
const width = 800;

Choose a reason for hiding this comment

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

ν˜Ήμ‹œ 이 κ°’ Cypress μ„€μ •κ°’ ν™˜κ²½λ³€μˆ˜κ°™μ€κ±°λ‘œ κ°€μ Έμ˜¬ 수 μ—†λ‚˜μš”? λ§Œμ•½μ— 뷰포트λ₯Ό λ‹€μ–‘ν•˜κ²Œ λŒλ €λ³Έλ‹€λ˜μ§€ ν•˜λŠ” μ‹μœΌλ‘œ ν…ŒμŠ€νŠΈ λ²”μœ„κ°€ ν™•μž₯되면 이 μ»€λ§¨λ“œλŠ” μ“°κΈ° μ–΄λ €μ›Œμ§‘λ‹ˆλ‹€.

Choose a reason for hiding this comment

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

그리고 κΆκ·Ήμ μœΌλ‘œλŠ” 이런 λ°©μ‹μœΌλ‘œ 계산을 ν•˜λŠ”κ²Œ 더 λ§žμ„κ±°κ°™μ€λ° ν•œλ²ˆ μ‹€ν—˜ν•΄λ³΄μ…”μš”.

function isInViewport(element) {
    const rect = element.getBoundingClientRect();
    return (
        rect.top >= 0 &&
        rect.left >= 0 &&
        rect.bottom <= (window.innerHeight || document.documentElement.clientHeight) &&
        rect.right <= (window.innerWidth || document.documentElement.clientWidth)
    );
}

return { computedTop, computedLeft };
}

private adjustPosition() {

Choose a reason for hiding this comment

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

λͺ¨μ–‘μƒˆλ§Œ λ΄μ„œλŠ” getComputedPos λŠ” adjustPos 의 둜컬 ν•¨μˆ˜λ‘œλ§Œ μžˆμ–΄λ„ λ˜κ±°λ‚˜ ꡳ이 λ‚˜λˆŒ ν•„μš”κ°€ μ—†μ–΄λ³΄μ΄λ„€μš”.


right -= parentOffsetLeft;
bottom -= parentOffsetTop;
} while (!includes(element.className.split(' '), cls('container')));

Choose a reason for hiding this comment

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

element.classList.contains λ₯Ό μ•ˆ μ“°λŠ” μ΄μœ λŠ” IE9 지원 λ•Œλ¬ΈμΌκΉŒμš”?

일단 μ§€κΈˆμ˜ λΉŒλ“œ ν™˜κ²½μœΌλ‘œλŠ” 폴리필이 μ•ˆλ“€μ–΄κ°€κ³  있기 λ•Œλ¬Έμ— μ•„μ‰½μ§€λ§Œ μ–΄μ©” 수 μ—†κΈ΄ ν•˜λ„€μš”.
차라리 κ·Έλ ‡λ‹€λ©΄ 일일이 className을 λ°°μ—΄λ‘œ μͺΌκ°œμ„œ μˆœνšŒμ‹œν‚€λŠ”κ±°λ³΄λ‹€ μ •κ·œν‘œν˜„μ‹μ„ ν™œμš©ν•˜λŠ”κ²Œ μ’€ 더 λ‚˜μ„ μˆ˜λ„ μžˆκ² μ–΄μš”.

@jajugoguma jajugoguma merged commit 599b533 into master May 26, 2022
@jajugoguma jajugoguma deleted the fix/context-menu-out-of-viewport branch May 26, 2022 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants