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.

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

Fix read this.worksheet before assign it #1934

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Fix read this.worksheet before assign it #1934

merged 2 commits into from
Apr 14, 2023

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
Siemienik
Siemienik approved these changes Apr 14, 2023
View reviewed changes
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
@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.

@motinados motinados mentioned this pull request Nov 18, 2023
Open
@j-mendez j-mendez mentioned this pull request Sep 8, 2024
Open
@Vishwas1 Vishwas1 mentioned this pull request Sep 8, 2024
Open
@j-mendez j-mendez mentioned this pull request Sep 13, 2024
Open
This was referenced Sep 15, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siemienik Siemienik Siemienik approved these changes

Assignees

@Siemienik Siemienik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZyqGitHub1 @hfhchan-plb @Siemienik