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: #1717 #1727

Open
wants to merge 1 commit into
base: master
from
Open

fix: #1717 #1727

wants to merge 1 commit into from

Conversation

@djrumph
Copy link

@djrumph djrumph commented Jan 23, 2020

I saw the issue was raised and a SheetJS Dev said the fix was simple and involved dealing with defined and undefined variables.
They wrote a line using a ternary conditional to assign the variable textp to the value of cell.v if it's defined, or an empty string if it is not defined.

SheetJS Dev Code: textp = cell.v == null ? "" : cell.v;

Suggesting using the line: textp = cell.v || "";

This does the same thing and assigns textp to cell.v if it is defined or an empty string if it is not.

(Note any feedback would be greatly appreciated, this is my first try at contributing to open source).

fix: #1717
I saw the issue was raised and a SheetJS Dev said the fix was simple and involved dealing with defined and undefined variables.
They wrote a line using a ternary conditional to assign the variable textp to the value of cell.v if it's defined, or an empty string if it is not defined. 

SheetJS Dev Code: textp = cell.v == null ? "" : cell.v;

Suggesting using the line: textp = cell.v || "";

This does the same thing and assigns textp to cell.v if it is defined or an empty string if it is not. 

(Note any feedback would be greatly appreciated, this is my first try at contributing to open source).
@SheetJSDev
Copy link
Contributor

@SheetJSDev SheetJSDev commented Jan 23, 2020

In JS there are 8 values that evaluate to false in a boolean context: undefined, null, 0, -0 (different from 0), 0n (BigInt 0), NaN, false and the empty string. The interesting (and as of yet unanswered) question is how to handle string cells with a numeric value of 0 or a boolean value of false. Your code would render both values as empty strings, whereas my code would render the numeric 0 as "0" and the boolean false as "false". Thoughts?

@djrumph
Copy link
Author

@djrumph djrumph commented Jan 24, 2020

Thank you for the great feedback and opportunity to dig into the code. I really appreciate your time.
So the goal is to only render an empty string if the value of cell.v is undefined, null or NaN, otherwise convert the value of the string to a primitive and assign it to textp?

If so here is my idea, feel free to skip over this if I interpreted what you meant wrong.

IDEA:
My thoughts are to use the original ternary statement combined with extra checks and a function to convert the cell.v value to a primitive.

Ultimately the code that passed all tests was:

textp = (cell.v === undefined || cell.v === null || isNan(cell.v.toString)) ? "" : JSON.parse(cell.v);

This code assigns the primitive value of the cell to the variable unless it is undefined, null or NaN.
I added a isNaN(cell.v.toString) to pass both the NaN test together. IsNaN() without toString() failed the 0n test.

The function JSON.parse turns a string into JSON, and for this purpose, it essentially turns strings into primitives.

END
Let me know if this addresses the issue you brought up or if I missed the mark.

Thank you again for the feedback. I really enjoy learning more by solving problems.

@SheetJSDev SheetJSDev force-pushed the SheetJS:master branch from 0786b99 to 3b589f0 Aug 12, 2020
@SheetJSDev SheetJSDev force-pushed the SheetJS:master branch from 4254ed4 to eec93b0 Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.