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

updates #488

Closed
wants to merge 2 commits into from
Closed

updates #488

wants to merge 2 commits into from

Conversation

ehallmark
Copy link
Contributor

First, we override stylesheet/num_fmt.rb in order to prevent the removal of underscores in number foramatting. (This solution was provided in a previous issue)

Second, we add attributes in the pivot_table class to handle pivotStyleInfo and whether row attributes ought to include the subtotal in the pivot table.

Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

This fails the tests.

str << "/>"
end
str << '</dataFields>'
end
# custom style
if style_info.present?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hash#present? is not defined when running the tests.

ActiveSupport is not available as a depdendency.

@ehallmark
Copy link
Contributor Author

thanks for pointing out the issue, it's fixed now

'<pivotField axis="axisRow" compact="0" outline="0" subtotalTop="0" showAll="0" includeNewItemsInFilter="1" defaultSubtotal="0">' + '</pivotField>'
else
'<pivotField axis="axisRow" compact="0" outline="0" subtotalTop="0" showAll="0" includeNewItemsInFilter="1">' + '<items count="1"><item t="default"/></items>' + '</pivotField>'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Style detail: Never use unless with an else. https://github.com/bbatsov/ruby-style-guide#no-else-with-unless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I'm used to Java which doesnt deal with unless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update based on ruby style guide

@olleolleolle
Copy link
Contributor

olleolleolle commented Nov 1, 2016

Perhaps you can improve the title of this PR, to make it work in a CHANGELOG document, if it ever gets that far?

elsif columns.include? cell_ref
'<pivotField axis="axisCol" compact="0" outline="0" subtotalTop="0" showAll="0" includeNewItemsInFilter="1">' + '<items count="1"><item t="default"/></items>' + '</pivotField>'
elsif pages.include? cell_ref
'<pivotField axis="axisCol" compact="0" outline="0" subtotalTop="0" showAll="0" includeNewItemsInFilter="1">' + '<items count="1"><item t="default"/></items>' + '</pivotField>'
'<pivotField axis="axisPage" compact="0" outline="0" subtotalTop="0" showAll="0" includeNewItemsInFilter="1">' + '<items count="1"><item t="default"/></items>' + '</pivotField>'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bug-fix, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct... will work on CHANGELOG

@randym
Copy link
Owner

randym commented Nov 3, 2016

@ehallmark as you make the changes requested by @olleolleolle please be sure to rebase and add specs covering you changes.

@ehallmark
Copy link
Contributor Author

@randym I rebased and added some specs here #499
I will close this PR now...

@ehallmark ehallmark closed this Nov 7, 2016
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