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: paste more rows than rows of the grid (fix #1635) #1685

Merged
merged 2 commits into from
May 27, 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 data was truncated when pasting more rows than the number of rows in the grid.
    • When calculating the range to be pasted during paste, if it exceeds the row range of the current grid, it adds as many blank rows as it exceeds and then performs pasting.
  • The test could not be added due to a Clipboard issue in Cypress.

As-Is

To-Be


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

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.

리뷰 완료합니다.

Comment on lines 125 to 130
if (endRowIndex > viewData.length - 1) {
appendRows(
store,
[...Array(endRowIndex - viewData.length + 1)].map(() => ({}))
);
}

Choose a reason for hiding this comment

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

저번 PR에 언급했던거랑 비슷하지만, 이 쿼리라는 모듈 영역에서 할 동작은 아닌거같아요.

  • 쿼리라는 이름의 특성 상 순수 함수이면 충분할 것 같은데 사이드 이펙트를 일으키고 있어요.
  • 디스패쳐가 쿼리를 의존하고, 쿼리가 디스패쳐를 의존하는 그닥 좋지 않은 의존 관계를 형성했어요. 가급적이면 의존성은 한 방향으로 흐르도록 만들어봅시다.

paste 함수에서 쿼리를 호출하고 있는데, 사전에 길이의 차이가 있는지 확인하고 Row를 늘려주는 동작은 여기서 수행한 뒤에 getRangeToPaste 를 호출해도 충분할 것 같아요.

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.

리뷰 완료합니다.

@jajugoguma jajugoguma merged commit 1fa2a9b into master May 27, 2022
@jajugoguma jajugoguma deleted the fix/paste-more-rows-than-grid branch May 27, 2022 08:28
@Bearsharks Bearsharks mentioned this pull request Oct 25, 2022
7 tasks
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.

3 participants