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.

Sign up for GitHub

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

Jump to bottom

feat: add defaultContent to summary options #252

Merged
merged 5 commits into from
Jan 24, 2019
Merged

feat: add defaultContent to summary options #252

merged 5 commits into from
Jan 24, 2019

Conversation

dongwoo-kim
Copy link

@dongwoo-kim dongwoo-kim commented Jan 21, 2019 •
edited

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

1. extends setSummaryColumnContent method

  • support object type in addition to string type for setting dynamic template
// 1. string (currently supported)
grid.setSummaryColumnContent('c1', 'This is C1 summary');

// 2. object (added in this PR)
grid.setSummaryColumnContent('c2', {
  template(value) {
    return 'Sum: ' + value.sum;
  }
});

2. extends summary.columnContent options

  • support string type in addition to object type for setting static content
var grid = new tui.Grid({
  // ...
  summary: {
    columnContent: {
      // 1. object (currently supported)
      c1: {  
        template(value) {
          return 'Sum: ' + value.sum;
        }
      },
      // 2. string (added in this PR)
      c2: 'This is C2 Summary'
    }
  }
})

3. adds summary.defaultContent options

  • set summary content for every column (shape of the option object is same as columnContent)
  • can be overridden for specific columns by columnContent options
var grid = new tui.Grid({
  // ...
  summary: {
    defaultContent: {
      template(value) {
        return 'Count: ' + value.count;
      }
    }
    columnContent: {
      c1: {  
        template(value) {
          return 'Sum: ' + value.sum;
        }
      }
    }
  }
})

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

@seonim-ryu
Copy link
Member

Can one of the admins verify this patch?

@seonim-ryu
Copy link
Member

ok to test

sohee-lee7
sohee-lee7 reviewed Jan 22, 2019
View reviewed changes
Copy link
Contributor

@sohee-lee7 sohee-lee7 left a comment

Choose a reason for hiding this comment

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

[1/22] 리뷰 완료입니다. 고생하셨습니다!

src/js/model/summary.js
*/
this.autoColumnNames = options.autoColumnNames;
this.autoColumnNameSet = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

결국 value는 true인 것들만 있는 것인데, 배열로 가지고 있으면 안되나요??
auto로 할 column들의 의미로.. [c1, c2] 이렇게 가지고 있도록이요.

Copy link
Member

Choose a reason for hiding this comment

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

음.. 연산이 복잡해져서 맵으로 관리하는거 아닐까요?
배열로 관리하면 값을 제거할 때 filter나 slice를 써야할테니까요 ;ㅅ;

Copy link
Author

Choose a reason for hiding this comment

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

네. 원래 배열이었는데, 순회할 일이 없고, 순서도 관계 없기 때문에 객체 형태로 사용하도록 변경했습니다.
배열은 아무래도 추가/삭제/중복확인 등을 할 때 불편해서요. (성능도 아주 미묘하게 느리구요)
ES6 환경이면 Set을 쓸텐데, Set이 없어서.. 그냥 객체 형태로 처리했습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

아하! 거기까진 생각 못했네요! 👍

seonim-ryu
seonim-ryu reviewed Jan 22, 2019
View reviewed changes
src/js/model/summary.js Outdated

/**
* Reset autoColumnNames and columnTemplateMap based on columnContent options.
* @param {Object} columnContent - summary.columnContent options
Copy link
Member

Choose a reason for hiding this comment

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

@private 추가요ㅎ

seonim-ryu
seonim-ryu reviewed Jan 22, 2019
View reviewed changes
src/js/model/summary.js Outdated
}, this);
},

/**
* Change Summary Value
* @param {string} columnName - Parameter description.
Copy link
Member

Choose a reason for hiding this comment

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

@private이 빠졌습니다ㅎ
그리고 Parameter description. 이건.. 잘못 들어간거죠?

Copy link
Author

Choose a reason for hiding this comment

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

JSDoc은 한 번에 넘어가는 일이 없군요 ㅎㅎ

seonim-ryu
seonim-ryu reviewed Jan 22, 2019
View reviewed changes
src/js/model/summary.js Outdated
* @private
*/
_isVisibleColumn: function(columnName) {
return this.columnModel.getVisibleColumns().indexOf(columnName) >= 0;
Copy link
Member

Choose a reason for hiding this comment

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

보통 indexOf() > -1 이렇게 비교하지 않나요?ㅎ

Copy link
Author

Choose a reason for hiding this comment

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

보통...은 잘 모르겠습니다만.. ㅋ
MDN에 includes 폴리필을 봤더니 indexOf() !== -1을 쓰는군요..
(이게 크기 비교보다는 아주 미묘하게 빠를지도 모르겠네요 ㅎ)

seonim-ryu
seonim-ryu reviewed Jan 22, 2019
View reviewed changes
test/unit/js/view/layout/summary.spec.js
});
summary.dimensionModel.set('summaryHeight', 30);
function createForRender(summaryOptions) {
var summary = create(frameConst.R, {
Copy link
Member

Choose a reason for hiding this comment

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

R사이드를 보니 문득.. L사이드에서도 동작 잘 하겠죠?ㅎ

Copy link
Author

Choose a reason for hiding this comment

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

L사이드는.. 고친 게 없어서. 잘 돌 거라 생각합니다 ㅎ

seonim-ryu
seonim-ryu approved these changes Jan 22, 2019
View reviewed changes
Copy link
Member

@seonim-ryu seonim-ryu left a comment

Choose a reason for hiding this comment

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

[1/22] 리뷰 완료하였습니다! 고생 많으셨습니다ㅎㅎ 역시 그리드 후계자 👍

@dongwoo-kim dongwoo-kim merged commit f9dacfb into master Jan 24, 2019
@dongwoo-kim dongwoo-kim deleted the feat/set-summary-column-content branch January 31, 2019 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sohee-lee7 sohee-lee7 sohee-lee7 left review comments

@seonim-ryu seonim-ryu seonim-ryu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dongwoo-kim @seonim-ryu @sohee-lee7