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

Add table footer support #114

Merged
merged 2 commits into from
May 1, 2018
Merged

Add table footer support #114

merged 2 commits into from
May 1, 2018

Conversation

gcotelli
Copy link
Member

Fixes #113

@gcotelli gcotelli requested a review from mtabacman April 30, 2018 14:15
@coveralls
Copy link

coveralls commented Apr 30, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 67b6d91 on table_footer_support into 4b7bf4c on master.

{ #category : #rendering }
WebTableColumnRenderer >> renderFooterCellSummarizing: tableContents on: aCanvas [

| heading |
Copy link
Member

Choose a reason for hiding this comment

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

heading should be footer here

Copy link
Member Author

Choose a reason for hiding this comment

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

done !

],
#category : #'Willow-WebViews'
}

{ #category : #'instance creation' }
TableWebView class >> definedBy: aColumnRendererCollection applying: aTableCommand headerRenderedBy: aHeader applyingToEachRow: aRowCommand [

^ self
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't like to have an explosion of instance creation methods. Tables should always be built with all parameters, and the table builders in each supplier should know to use the null defaults if nothing is specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will go that route but we need to deprecate the previous ones first.. or release a major version ... Maybe we can do that as a two step process? I would like to review the methods in the builder also, because essentially are 3 aspects to consider now: headers, column definitions and footers.. By now I've just added the more generic method with footer support because if we start making each combination it will be an explosion. But now the builder it's a bit verbose for the simple cases with footers :( . What do you think? Release it as is and open an issue to remove the extra instance creation methods and improving the table builder API, or wait for this changes to be made?

Copy link
Member

@mtabacman mtabacman left a comment

Choose a reason for hiding this comment

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

Ok, we'll go with the two-step improvement.

@gcotelli gcotelli mentioned this pull request May 1, 2018
@gcotelli gcotelli merged commit 6c605e7 into master May 1, 2018
@gcotelli gcotelli deleted the table_footer_support branch May 1, 2018 12:19
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

3 participants