Skip to content

Fixed conditional format corrupting sheet #1305 #1574

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

Merged

Conversation

rolandostar
Copy link
Contributor

Summary

I'm using a template for styling, and filling out data with ExcelJS. When my template had conditional formatting enabled on it, opening it and writing the file (even without data being inserted) would corrupt the file and lose styling. While investigating the issue I ran into this comment I tried it and it worked, and since I wasn't able to find a PR mentioning this fix I decided to open one myself.

Test plan

My original template file's conditional formatting:

After being loaded by ExcelJS and rewritten into a different file:

image
image

Notes

I understand the README.md mentioned writing tests for all fixes but I'm not really familiar with it, I can write them if someone could point me in the right direction.

@mitchellwagner
Copy link

I can confirm that this fixes the issue with conditional styling and the corruption of the worksheet

Copy link
Member

@Siemienik Siemienik left a comment

Choose a reason for hiding this comment

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

I confirm that the new order is correct:
https://github.com/exceljs/ooxml-xsd/blob/aba7f40dc52d9a3cfe236e9e3e15a5148d5ca999/sml-styles.xsd#L830-L868

Thank you for that great contribution! 🥇 LGTM.

@Siemienik Siemienik merged commit 2ab468b into exceljs:master Apr 11, 2021
@exceljs exceljs locked and limited conversation to collaborators Apr 11, 2021
@rolandostar rolandostar deleted the fix/conditional-format-corrupt branch July 26, 2021 02:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants