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 style merging #15

Merged
merged 3 commits into from Sep 13, 2016

Conversation

Projects
None yet
2 participants
@westonganger
Copy link
Contributor

commented Aug 22, 2016

Addresses issue #14

It now only safely patches Axlsx::Styles.add_style to implement a style cache. Style index had to be moved from the Workbook to Styles because you cant access the workbook from the add_styles method. Tests are all passing but new ones added for this probably aren't super meaningful yet.

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2016

This is looking very good! I'll try to take a closer look this or next week.

sheet.add_style 'B2:D4', b: true
sheet.add_style 'B2:D4', border: { style: :thin, color: '000000' }
end
@workbook.apply_styles

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

Could you replace this line with serialize(filename) and replace @filename on line 242 with filename?

I forgot to remove @filename in all places when I was removing tests "teardown" hook that was using the variable. The reason I'm asking to add serialize is for visual inspection of test results in the tmp folder.

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

I see that both bold font face and border are being applied which is good. Unfortunately, the border is being drawn around every cell. In the original implementation the border is only added to the interior of the selected range (as in the example on the readme).

border_issue

This comment has been minimized.

Copy link
@sakovias

sakovias Sep 4, 2016

Collaborator

Actually, I was wrong on my previous comment:

sheet.add_style 'B2:D4', border: { style: :thin, color: '000000' }

does exactly what it's supposed to do, which is apply border to every cell in the range. This is how axlsx works. The default border is a border applied to every edge of the cell.

sheet.add_row ['A2', 'B2']
sheet.add_border 'A1:B2'
end
@workbook.apply_styles

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

Same here please

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

The border is added correctly here, but the bold face on B1 is getting lost.

bold-issue

sheet.add_row ['A2', 'B2']
sheet.add_style 'B1:B2', bg_color: 'ff0000'
end
@workbook.apply_styles

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

And here too please

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

The bold face got lost here as well.

bold-issue-2

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2016

I like how clean this change is looking and this seems to be very close to the end goal 👍

Lets make sure the new tests that you've added produce the results we want. I'll add some comments in the test file. Let me know if you need any help.

@@ -9,6 +9,6 @@ def test_can_add_style
cell = row.cells.first

cell.add_style b: true
assert_equal({ b: true }, cell.raw_style)
assert_equal({b: true}, cell.raw_style)

This comment has been minimized.

Copy link
@sakovias

sakovias Aug 27, 2016

Collaborator

Very minor nitpick... I think having spaces inside curly braces is a popular Ruby tradition. Rubocop (with default config) likes that too. There are many other Rubocop warnings here though which is all my fault 😄 (most of this was written before I started using Rubocop).

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2016

I think the Hash method key might not be doing a good job comparing hashes I am going to change this and see what happens.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

Okay so I fixed the style merging in the latest commit. I had forgotten to add the cell styles from the index to the raw_styles before applying it to the cell.

As for the borders issue, I noticed that for test_merge_styles_1 that the problem was fixed when I switched from:
sheet.add_styles 'B2:D4', borders: { style: :thin, color: '000000' }
to this:
sheet.add_border 'B2:D4', { style: :thin, color: '000000' }

I would appreciate your insight as to why the first statement is failing.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

Actually I just checked with the current release v0.1.5 and the sheet.add_styles 'B2:D4', borders: { style: :thin, color: '000000' } is broken here as well. I suspect that you have incorrect documentation regarding this or add_styles need to be fixed to use the BorderCreator.

end
return index
end

This comment has been minimized.

Copy link
@sakovias

sakovias Sep 4, 2016

Collaborator

Is this just in case or a leftover?

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2016

I'm really glad you've fixed it! I'll take a closer look some time soon.

I apologise that I was wrong about the border. If we use

sheet.add_border 'B2:D4', { style: :thin, color: '000000' }

this goes to border creator which only puts up an outer border. But when we use

sheet.add_styles 'B2:D4', borders: { style: :thin, color: '000000' }

this would apply the given style to every cell in the range, i.e. put border around each cell in B2:D4. I think when I was making comments last time I misread add_styles for add_border. Sorry about that.

sheet.add_row ['', '1', '2', '3']
sheet.add_row ['', '4', '5', '6']
sheet.add_row ['', '7', '8', '9']
sheet.add_style 'B2:D4', b: true

This comment has been minimized.

Copy link
@sakovias

sakovias Sep 6, 2016

Collaborator

Could you extract "bold" as a style, so that we test merging old styles and new styles (the same way as you do in the two tests that follow)?

bold = @workbook.styles.add_style b: true
# ...
sheet.add_row ['', '1', '2', '3'], style: [nil, bold, nil, nil]
@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2016

I think this is good to go. Good job!

There's one small comment that I've left in the tests (re: testing merge of old and new styles). We can merge when that's cleared.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2016

Alright I have added those changes. With these new features maybe you want to bump this gem to v1.0.0

@@ -2,3 +2,5 @@
require 'minitest/autorun'
require 'minitest/pride'
require 'awesome_print'

mkdir_p(File.expand_path("../../tmp", __FILE__))

This comment has been minimized.

Copy link
@sakovias

sakovias Sep 13, 2016

Collaborator

Thanks for adding this!

@sakovias

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2016

Well done! Thank you for your contribution!

@sakovias sakovias merged commit 3b96ca3 into axlsx-styler-gem:master Sep 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.