Skip to content

add xml:space="preserve" for all whitespaces #896

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
merged 5 commits into from
Oct 18, 2019

Conversation

sebikeller
Copy link
Contributor

@sebikeller sebikeller commented Jul 23, 2019

Currently only spaces at begin or end of a text add the xml:space attribute.
So at the moment comments with newlines can not be added.

Currently only spaces at begin or end of a text add the xml:space attribute. 
So at the moment comments with newlines can nott be added.
@Siemienik
Copy link
Member

@sebikeller could I ask you to write a test that covers this change?

@guyonroche
Copy link
Collaborator

@sebikeller - I've just tried adding some integration tests to master that add white spaces to values and notes at beginning and endings of multiple lines and I can't reproduce your problem.
If you could add an integration test that fails without your change we can review this PR better.
Thanks,

Sebastian Keller added 4 commits October 17, 2019 00:54
Test generates an Excel File with a cell and a cell note containing multiline RichText endign in `\r\n`
@sebikeller
Copy link
Contributor Author

sebikeller commented Oct 16, 2019

I agree, with an integration test, its not possible to reproduce the problem.

But if you open the generated file with excel, you'll see the difference.

branch master

master

exceljs-master.xlsx

applied pr

out pr896

exceljs-pr896.xlsx

@sebikeller
Copy link
Contributor Author

sebikeller commented Oct 17, 2019

Recreating the file in excel, following xml can be extracted:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<comments xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
  <authors>...</authors>
  <commentList>
    <comment ref="A1" authorId="0" shapeId="0">
      <text>
        <r>
          <rPr>
            <b/>
          </rPr>
          <t>First Line:</t>
        </r>
        <r>
          <t xml:space="preserve">
</t>
        </r>
        <r>
          <t xml:space="preserve">Second Line
</t>
        </r>
        <r>
          <t xml:space="preserve">Third Line
</t>
        </r>
        <r>
          <t>Last Line</t>
        </r>
      </text>
    </comment>
  </commentList>
</comments>

@sebikeller
Copy link
Contributor Author

Since we cant recreate it with an integration test, it actually means, that the exceljs parser has difference with that of excel.
So you might consider adding a trim() to text values unless xml:space="preserve" is set.

@guyonroche
Copy link
Collaborator

@sebikeller - agreed! I'll add a unit test on the strings xforms to keep a check on this...

@guyonroche guyonroche merged commit fb37116 into exceljs:master Oct 18, 2019
@sebikeller sebikeller deleted the patch-1 branch February 3, 2020 08:29
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.

3 participants