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

Fix read this.worksheet before assign it #1934

Merged
merged 2 commits into from Apr 14, 2023
Merged

Conversation

ZyqGitHub1
Copy link
Contributor

The anchor.js assign this.worksheet = worksheet at end of constructor, but the getter (such as rowHeight) used in constructor will read this.worksheet. so it will cause rowHeight fall back to default value (180000);

Summary

When I want to add a image in a cell over a range, and I adjust the height of this row before. The nativeRowOff will not calculate use the row height.

const Excel = require('exceljs');
const fs = require('fs').promises;

(async () => {
    const workbook = new Excel.Workbook();
    await workbook.xlsx.readFile('./test.xlsx');
    const worksheet = workbook.getWorksheet(1);
    const row = worksheet.getRow(1);
    row.height = 400;
    const col = worksheet.getColumn('A');
    col.width = 200
    const image = await fs.readFile('./image.png')
    const imageId = workbook.addImage({
        buffer: image,
        extension: 'png',
    });
    worksheet.addImage(imageId, {
        tl: { col: 0.5, row: 0.5 },
        br: { col: 1, row: 1 }
    });
    await workbook.xlsx.writeFile('./result.xlsx');
})()

result is
图片

Test plan

After I change anchor.js,

const Excel = require('exceljs');
const fs = require('fs').promises;

(async () => {
    const workbook = new Excel.Workbook();
    await workbook.xlsx.readFile('./test.xlsx');
    const worksheet = workbook.getWorksheet(1);
    const row = worksheet.getRow(1);
    row.height = 400;
    const col = worksheet.getColumn('A');
    col.width = 200
    const image = await fs.readFile('./image.png')
    const imageId = workbook.addImage({
        buffer: image,
        extension: 'png',
    });
    worksheet.addImage(imageId, {
        tl: { col: 0.5, row: 0.5 },
        br: { col: 1, row: 1 }
    });
    await workbook.xlsx.writeFile('./result.xlsx');
})()

the result is
图片
The image top-left will start from 0.5*row height

Related to source code (for typings update)

https://github.com/ZyqGitHub1/exceljs/blob/458f372609589e6b3665a5c955998b677df2037e/lib/doc/anchor.js#L7

@Siemienik Siemienik self-assigned this Apr 6, 2023
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.

Good catch! LGTM 🥇

@Siemienik Siemienik merged commit 5f306b6 into exceljs:master Apr 14, 2023
23 of 24 checks passed
@hfhchan-plb
Copy link
Contributor

This change means that all of the existing images are now off because the actual row height and col width are now being used. The previous behaviour was arguably a bug, but this is a breaking change causing every single image to be positioned differently.

@hfhchan-plb
Copy link
Contributor

There really should be an API which allows us to do actual absolute positioning, i.e. fixed pixel offset from the cell or fixed pixel offset from top left of spreadsheet, instead of the current absolute positioning which is more like relative.

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.

None yet

3 participants