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

feat: add APIs that return added class names #1561

Merged
merged 4 commits into from
Jan 21, 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

  • Added APIs to return added class names.
    • In the case of a cell, the class name added to the column and row to which the cell corresponds is also returned.
    • In the case of a column, only the class names added to the column in all rows are returned.

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

Copy link

@jwlee1108 jwlee1108 left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.

const { rawData } = this.store.data;
const classNamesOfFirstRow: string[] = rawData[0]._attributes.className.column[columnName];

if (!isEmpty(classNamesOfFirstRow)) {
Copy link

@jwlee1108 jwlee1108 Jan 21, 2022

Choose a reason for hiding this comment

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

확실히 calendar에서 쓰는 isPresent 같은 긍정 조건이 더 좋아보이긴 하네요.

(+추가)

혹시 이런 건 어떨까요..?

if(isEmpty()) {
  return [];
}

return rawData.slice(1).reduce();

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.

리뷰 완료합니다.

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.

리뷰 완료합니다.

return rawData.slice(1).reduce((acc, row, _, arr) => {
const classNames = row._attributes.className.column[columnName];

if (isEmpty(classNames) || isEmpty(acc)) {

Choose a reason for hiding this comment

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

isEmpty(acc) 부분은 위에서 이미 걸러지는거 아닌가요?
그리고 아래에서 arr.splice(0) 를 하게되는 이유가 궁금해지네요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아래쪽의 return acc.filter((className) => includes(classNames, className));을 보시면 아시겠지만, 기본 동작이 첫 행이 가지고 있는 ClassNames에서 다른 행들은 가지고 있지 않는 ClassNames를 제외하는 방식으로 구현 되어있습니다.

이 과정에서, 만약 모든 ClassName이 제거된다면(컬럼에 할당한 ClassName이 없다면) 더 이상 loop를 수행할 이유가 없어져서 이를 확인하기 위해 isEmpty(acc)를 추가했습니다.

그리고 Array.reduce는 네 번재 인자로 전달되는 배열에 대해 수행되기 때문에 더 이상 loop를 수행할 필요가 없어진 경우 이 배열을 변형해 조기 종료를 수행하기 위해 arr.splice(0)를 수행했습니다.

@jajugoguma jajugoguma merged commit 8f137b8 into master Jan 21, 2022
@jajugoguma jajugoguma deleted the feat/get-added-class-names branch January 21, 2022 08:45
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.

4 participants