fix: missing PRN in xlsx.mini #1908

Merged
chenxeed merged 1 commits from chx/fix-mini-prn into master 2020-06-25 19:32:51 +00:00
chenxeed commented 2020-04-20 07:02:41 +00:00 (Migrated from github.com)

Issue

https://github.com/SheetJS/sheetjs/issues/1760

Description

Based on my investigation, it seems like the function read_prn in the mini is missing PRN when it tries to read a string of CSV file.

My proposed solution is to add the PRN declaration as part of the mini build, which exists in the bits/40_harb.js.

# Issue https://github.com/SheetJS/sheetjs/issues/1760 # Description Based on my investigation, it seems like the function `read_prn` in the mini is missing `PRN` when it tries to read a string of CSV file. My proposed solution is to add the `PRN` declaration as part of the `mini` build, which exists in the `bits/40_harb.js`.
chenxeed commented 2020-04-20 07:45:10 +00:00 (Migrated from github.com)

@SheetJSDev I have no written unit test in this changes, but I have tested the changes in my local application and the CSV works. Also, the existing unit test seems to pass when I run the test locally.

test import svg

@SheetJSDev I have no written unit test in this changes, but I have tested the changes in my local application and the CSV works. Also, the existing unit test seems to pass when I run the test locally. ![test import svg](https://user-images.githubusercontent.com/3530355/79726711-ce7fe580-831d-11ea-8dc7-ac689f35907f.gif)
chenxeed commented 2020-05-20 07:12:04 +00:00 (Migrated from github.com)

Poke @SheetJSDev again 🙇 it would be great to have this settled for the next release, so we can use xlsx.mini in our application

Poke @SheetJSDev again 🙇 it would be great to have this settled for the next release, so we can use `xlsx.mini` in our application ☕
chenxeed commented 2020-06-16 07:21:07 +00:00 (Migrated from github.com)

Hi @SheetJSDev @srijonsaha may I have an update or response for the PR?

If there's anything missing from the PR, please let me know to improve.

I'm waiting for this patch before I can use xlsx.mini in my production project.

Thanks!

Hi @SheetJSDev @srijonsaha may I have an update or response for the PR? If there's anything missing from the PR, please let me know to improve. I'm waiting for this patch before I can use `xlsx.mini` in my production project. Thanks!
SheetJSDev commented 2020-06-16 07:24:09 +00:00 (Migrated from github.com)

@garrettluu can you take a peek?

@garrettluu can you take a peek?
garrettluu (Migrated from github.com) requested changes 2020-06-16 18:48:09 +00:00
garrettluu (Migrated from github.com) left a comment

Code itself looks fine, but please squash your commits into 1 with git rebase, since we don't want too many commits to clutter the history.

Code itself looks fine, but please squash your commits into 1 with `git rebase`, since we don't want too many commits to clutter the history.
chenxeed commented 2020-06-18 05:53:41 +00:00 (Migrated from github.com)

Thanks @garrettluu for the response and review. I've squashed the commit. Look forward to the update 👍

Thanks @garrettluu for the response and review. I've squashed the commit. Look forward to the update 👍
chenxeed commented 2020-06-25 05:09:58 +00:00 (Migrated from github.com)

Hi @garrettluu @SheetJSDev I've made the requested changes. Please confirm if this is fine. Look forward to the release! 🙇

Hi @garrettluu @SheetJSDev I've made the requested changes. Please confirm if this is fine. Look forward to the release! 🙇
garrettluu (Migrated from github.com) approved these changes 2020-06-25 19:32:29 +00:00
Sign in to join this conversation.
No description provided.