Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Improvements for images (correct reading/writing possitions)#702

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
guyonroche merged 4 commits intoexceljs:masterfromp3rio:master
Apr 1, 2019

Conversation

@Siemienik
Copy link
Member

@SiemienikSiemienik commentedDec 1, 2018•
edited
Loading

Improvements for image`s positioning/stretching and their properly writing to xlsx and readign from xlsx with fully backward compatibility.

Before that changes,

Reading images positions workswrong. This example show it very well,
original file looks that:
image
after i open this file and savewithout any changes:
image

Backward compability

My solution was designed to handle currentcol/row and calculate it tonativeCol,nativeColOff,nativeRow,nativeRowOff. A lot of tests was written to ensure properly working with dependents apps .

I will be glad for quick checking/approval this pull request, I am open for your comments and ready to make some fixes.

issues

#650
#467

jeremyjh, moddaman, and kirillkh reacted with thumbs up emojimoddaman reacted with hooray emoji
…g from xlsx with fully back compatible.NEW Anchor modelNEW Integration tests test for image`s reader & writerNEW Unit tests for Anchor
@SiemienikSiemienik changed the titleImprovements for images (correct reading/writing stream)Improvements for images (correct reading/writing possitions)Dec 1, 2018
@Siemienik
Copy link
MemberAuthor

Siemienik commentedJan 10, 2019•
edited
Loading

Hi everyone, I didn't note any maintainer's activity, so as long as it's required I am going to maintain this wonderful open source library in my fork:https://github.com/p3rio/exceljs

I hope that, in the future both repositories become to be merged ;)

Copy link
Member

@alubbealubbe left a comment

Choose a reason for hiding this comment

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

I don't really know enough about images in excel, but I like the tests, so I think this can be merged. Should we maybe do a rebase?

this.autoFilter=value.autoFilter;
this._media=value.media;

varself=this;
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is not needed

@@ -0,0 +1,56 @@
/**
* Copyright (c) 2018 Paweł Siemienik
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I think this shouldn't be in individual files

Copy link
MemberAuthor

@SiemienikSiemienikJan 15, 2019•
edited
Loading

Choose a reason for hiding this comment

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

@alubbe In each file is copyright/author block similar to this, if it's wrong could you write how it should looks like?

Copy link
Member

Choose a reason for hiding this comment

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

ah indeed - I had completely missed that. Let's leave that in then.

@guyonroche I've only seen licenses repeated in every file if you want to charge license fees for your IP, but since we use MIT I think there's no need to have this repeated everywhere - it's just noise that needs maintenance. Would you be okay with removing them everywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alubbe Yeah - We could probably get away with just the LICENCE file with links from the README.md and main excel.js file.
I finally have more spare time now so I can start working through the PRs. I'll test this one tomorrow morning

@Siemienik
Copy link
MemberAuthor

Thank You@alubbe for review, I will make required fixes as soon as possible ;)

@alubbe
Copy link
Member

Happy now with this PR code-wise, but I can't speak to the functionality. Can anyone test it with an actual excel file?

@Siemienik
Copy link
MemberAuthor

I had to made this fixes for my businesses project when I read and modify xlsx files. With this PL it works fine.

below is link to xlsx file, what i prepared for test purposes. If you have possible, just download, open&write it (exceljsv1.7.0):
https://github.com/exceljs/exceljs/blob/c8e2ce5190a3b627a33c90a7fee1358351443440/spec/integration/data/images.xlsx

moddaman reacted with thumbs up emoji

@moddaman
Copy link

nice@Siemienik

Looking to forward to this being merged. Until then i have updated my package.json to use `"exceljs": "github:p3rio/exceljs#build"``.

How do i use this functionality? Do I use it the same way?
In your commits I can see som use of the Anchor class?

Could you update the readme?

@kaidoj
Copy link

kaidoj commentedFeb 13, 2019•
edited
Loading

This did not work for me. I tested my excel in 3 different Excel readers and it only worked in one. Images where placed wrong. For now i went back to my fork. That atleast works with my offsets set.

tl:{col:imageColumnPosition+0.05,row:i+0.05,},br:{col:imageColumnPosition+0.95,row:i+0.95//this aint working either. I have to use 1 but that is bad. Image overlapping row line.}

@Siemienik
Copy link
MemberAuthor

@kaidoj could You share excel files which were failed?

@Siemienik
Copy link
MemberAuthor

@moddaman it enough to use images like before. This PR fixes counting possitions for images in reader and writer. Before that, reader and writer ignore current column width and always use the same value, so when column width was larger than 'this const' it put image to another column.

moddaman reacted with thumbs up emoji

@alubbe
Copy link
Member

@kaidoj could you please share a file where it doesn't work?
@Siemienik why are the tests red - is our test runner broken? :D

@dogusev
Copy link
Contributor

@alubbe
Any chance to have it merged?

@alubbe
Copy link
Member

@alubbe Yeah - We could probably get away with just the LICENCE file with links from the README.md and main excel.js file.
I finally have more spare time now so I can start working through the PRs. I'll test this one tomorrow morning

@guyonroche awesome, will be great to have you back! :)

@mjiskinng
Copy link

Hi Guyz how to solve this issue. Any idea if i add dependencies "exceljs": "github:p3rio/exceljs#build" it still throws error i believe its clashing with the latest exceljs. Any idea how can i work with both in parallel using image functionality of p3rio and rest from master. Please help me this issue is now taking more than 1 month..

@Siemienik
Copy link
MemberAuthor

Siemienik commentedJul 26, 2021•
edited
Loading

Hi Guyz how to solve this issue. Any idea if i add dependencies "exceljs": "github:p3rio/exceljs#build" it still throws error i believe its clashing with the latest exceljs. Any idea how can i work with both in parallel using image functionality of p3rio and rest from master. Please help me this issue is now taking more than 1 month..

@mjiskinng Since 1 Apr 2019 this PR is included in the ExcelJS main package. Could you use :
"exceljs":"^4.2.1" ?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@alubbealubbealubbe left review comments

@guyonrocheguyonrocheAwaiting requested review from guyonroche

@defshiftdefshiftAwaiting requested review from defshift

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Siemienik@alubbe@moddaman@kaidoj@dogusev@mjiskinng@guyonroche
[8]ページ先頭
©2009-2025 Movatter.jp