-
Notifications
You must be signed in to change notification settings - Fork 422
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
!rows array, page view & margins, image support #19
base: master
Are you sure you want to change the base?
Conversation
@protobi if the images part of this PR were extracted and made into another PR. Would you accept it? |
@Siguza how functional were your changes before the conflict? |
Fully functional, I'd say. Limited, but functional. For image support, using the dist from my fork you should be able to literally copy the last code block from my PR, replace the Everything else is fairly simple and only required a handful of XML tags or attributes to be added or set. |
Thanks a lot! |
The image handling would be a great feature (It's sad, but our client uses images on excel files). Do you guys have any plan on fixing + merging this branch soon? Or to separate the image development in another PR? Thank you all very much for this awesome module! |
Is this project dead? I've got a pretty serious issue and it doesn't seem anyone is even replying to issues anymore. (#39) |
Well I'm still around, but I only forked the repo and hacked image support together. |
Well I wouldn't actually mind trying to write the fix myself if I could figure out what exactly is causing the issues. The only reason I'm hesitant to provide the before/after .xlsx is because it contains government records. However, if you can get me a general idea of where the issue may be occuring, I'd love to try to fix it myself. I think I provided as much info as I could get under my issue (linked in the prior comment) |
The most likely source of a bug in worksheets/sheetX.xml would seem bits/67_wsxml.js. |
I'd love to support the project more. The challenge for me is that the base project is silent. There are tens of thousands of unit tests for the server and browser, and these catch a surprising number of issues with seemingly simple edits. So I'm hesitant to accept PRs without running the tests. However, the base project combined XLSX with XLS, XLSB and XLSM into one project and some of these no longer work in Node 4+. Ideally, the base project would work with Node 4+, I pull those changes in and roll along. I've been hoping that would happen by now. Even better would be for the base project to handle styles, and we all support that. I think the root issue here is the economics of open source which requires one small group with the skill and intensity to manage the core. I can make the business case to support the styles extension but not the XLS/XLSB or node migration part. |
Yeah that makes sense to me. Unfortunately I don't feel like I have the expertise to get my hands really dirty with these types of migrations/updates. I really wish the base project wasn't dead. This seems like a very useful project that just kind of died. |
@Siguza we were looking at an issue SheetJS#509 which appears to have plucked changes from your PR. Nice work! We already pulled in the core of the changes from @paulish PR but have yet to finalize the row parameters. With regards to the page margins, is there any particular reason for the defaults you chose? The UI for Excel 2007 and Excel 2016 both quote rounded decimals: And for completeness sake, here are a few sample files (and they show the same numbers as the UI): Since the file has no conception of "wide/narrow/normal", only the raw numbers, it makes sense to merely document the sizes in the README but default to the normal values. |
@SheetJSDev: |
@Siguza in Excel, if you go to page layout and hit the margins button, you'll see a dropdown. Can you read off what is written there? The margins according to your files are:
|
The values don't match my current ones.
This is a fresh install, so my old values were most likely custom. |
@Siguza we just pushed a change in the main repo to add page margin parsing for the main formats as well as page margin writing for XLSB: SheetJS@1587688#diff-d3b36db34e6b01d5a4720fd524e20279. If you can fork the main repo and add in the XLSX write stuff, we'd accept a PR. Also check your email :) |
changes distilled from Siguza/js-xlsx see protobi#19 for discussion
Wanted to add I worked a bit on images -- I needed to be able to add multiple and set some properties (offsets and sizes). |
changes distilled from Siguza/js-xlsx see protobi/js-xlsx#19 for discussion
changes distilled from Siguza/js-xlsx see protobi#19 for discussion
changes distilled from Siguza/js-xlsx see protobi/js-xlsx#19 for discussion
Credit: !rows handling: paulish
View usage:
Margin usage:
Images usage: