Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

fix: OPTIC-152: Uploading a text field but only 'true' is showing up not 'false' #294

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

yyassi-heartex
Copy link
Contributor

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Frontend

Describe the reason for change

there was an issue where all falsey values would be treated as empty string, which is undesired. this change makes it so 0 and false will render as expected but null and undefined will be treated as empty string

Comment on lines 7 to 8
/* if undefined or null we'll treat it as empty string, otherwise toString(), this will allow 0 and false to render properly */
return (value ?? "").toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

the same comment as in #295:

what kind of value could be here? it won't fail on null or undefined and that's the only values triggering nullish coalescing.

the issue with false and 0 is fully fixed by next change. but most probably it won't work as expected if you want to return empty string for null and undefined, then you have to check for them specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied a change based on our discussion. i also added a unit test in LS PR

@yyassi-heartex yyassi-heartex merged commit 0df716f into master Jan 30, 2024
5 checks passed
@yyassi-heartex yyassi-heartex deleted the fb-optic-152/string-cell branch January 30, 2024 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants