Skip to content

fix issue #1045 horizontalCentered & verticalCentered in page not working #1073

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.

Sign up for GitHub

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

Jump to bottom
Merged
Siemienik merged 4 commits into from
Jan 17, 2020
Merged

fix issue #1045 horizontalCentered & verticalCentered in page not working #1073

Siemienik merged 4 commits into from
Jan 17, 2020

Conversation

@Alanscut
Copy link
Member

@Alanscut Alanscut commented Jan 15, 2020

Changes

  1. This PR solves the problem that horizontalCentered & verticalCentered is not working.

  2. Fix for horizontalCentered & verticalCentered in page not working #1045

@Alanscut Alanscut requested a review from Siemienik January 15, 2020 11:37
@Siemienik
Copy link
Member

Siemienik commented Jan 15, 2020

@Alanscut thanks for that PR, it's a bit of great work here:)
Could I ask you to write at least one test which checks this logic? (for example, write and read file + asserts for these properties).

@Alanscut
Copy link
Member Author

Alanscut commented Jan 16, 2020

hi @Siemienik
Thank you very much for your comment.
See https://github.com/exceljs/exceljs/blob/master/spec/unit/xlsx/xform/sheet/print-options-xform.spec.js#L23

@Siemienik
Copy link
Member

Siemienik commented Jan 16, 2020

hmm.. so how the tests work ok before and now after source code was changed? Are both versions correct?

@Alanscut
Copy link
Member Author

Alanscut commented Jan 17, 2020

@Siemienik Having researched the unit test, I found I have misunderstood it. Actually the testcases for it lies in file worksheet-xform.spec.js, property horizontalCentered are not tested before, so I have add the test case and fixed issue #1079 also.

exceljs/spec/unit/xlsx/xform/sheet/worksheet-xform.spec.js

Lines 53 to 70 in 9054c32

{
title: 'Sheet 1',
create: () => new WorksheetXform(),
initialModel: fixDate(require('./data/sheet.1.0.json')),
preparedModel: fixDate(require('./data/sheet.1.1.json')),
xml: fs.readFileSync(`${__dirname}/data/sheet.1.2.xml`).toString(),
parsedModel: require('./data/sheet.1.3.json'),
reconciledModel: fixDate(require('./data/sheet.1.4.json')),
tests: ['prepare', 'render', 'parse'],
options: {
sharedStrings: new SharedStringsXform(),
hyperlinks: [],
hyperlinkMap: fakeHyperlinkMap,
styles: fakeStyles,
formulae: {},
siFormulae: 0,
},
},

@Siemienik
Copy link
Member

Siemienik commented Jan 17, 2020

great :)

@Siemienik Siemienik merged commit 207ba4e into exceljs:master Jan 17, 2020
@Alanscut Alanscut deleted the issue_1045 branch May 5, 2020 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siemienik Siemienik Awaiting requested review from Siemienik

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Alanscut @Siemienik