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

[Excel 2003 XML] Add support for font colors + required fixes #45

Merged
merged 2 commits into from
Feb 22, 2018

Conversation

ms-ati
Copy link
Contributor

@ms-ati ms-ati commented Feb 15, 2018

As mentioned in #42, all I'd needed was to add support for the color of a given cell. But unfortunately, I discovered that other pieces of the puzzle were broken along the way.

This PR attempts to fix all that, adding the support for fonts and colors via #font.

Adds RSpec coverage via the following test files (screenshot included for those without Excel):
font_colors.xml.zip

font_colors_screenshot_in_mac_excel_16 10

Fixes #41, #42, #43, #44

Also fixes #33 I believe

@ms-ati
Copy link
Contributor Author

ms-ati commented Feb 15, 2018

Following semantic versioning, I recommend bumping a MINOR version, to 1.2.0 after this PR is merged, as this code adds new capability (font name, size, and color) into an existing API, and fixes existing bugs.

However -- there may be users depending on the mis-parsing of the cells, especially row and column numbers around merged cells. Therefore, you may need to bump a MAJOR version, to 2.0.0, to reflect incompatible changes for some users.

Does that make sense? Depends how you all are thinking about it.

@coveralls
Copy link

coveralls commented Feb 15, 2018

Coverage Status

Coverage increased (+1.2%) to 80.977% when pulling 5a06c76 on ms-ati:excel-2003-xml-support-font-colors into 00ccb4c on roo-rb:master.

@ms-ati
Copy link
Contributor Author

ms-ati commented Feb 21, 2018

@Empact et all -- any chance of getting eyes on this PR?

@Empact Empact merged commit 7a7e715 into roo-rb:master Feb 22, 2018
@Empact
Copy link
Contributor

Empact commented Feb 22, 2018

This looks great. Thanks! Keep the PRs coming. :)

@ms-ati
Copy link
Contributor Author

ms-ati commented Feb 22, 2018

Hurray! Thanks @Empact. I might try to tackle the streaming api question, now that you mention it. In the meantime -- our team would really appreciate it if you could cut a gem release that contains this PR?

Cheers!

-Marc

@ms-ati
Copy link
Contributor Author

ms-ati commented Feb 26, 2018

Ping on request for a roo-xls gem release @Empact ?

@Empact
Copy link
Contributor

Empact commented Feb 26, 2018

@ms-ati Released! Could you give it a try? https://rubygems.org/gems/roo-xls/versions/1.2.0

Also a github release /~https://github.com/roo-rb/roo-xls/releases/tag/v1.2.0

@ms-ati
Copy link
Contributor Author

ms-ati commented Feb 26, 2018

Nice @Empact ! Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants