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 bug in Cadence Web where falsy values do not render correctly #529

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

adhityamamallan
Copy link
Contributor

@adhityamamallan adhityamamallan commented Nov 15, 2023

Summary

This change fixes an issue with cadence-web where false/0 values in the activity result do not render correctly. This is due to a check in the getJsonStringObject helper function returning an empty string if the result is falsy. While this is the intended behaviour for null/undefined, we want to render other values (such as false and 0) correctly.

This change makes the check for null/undefined in the getJsonStringObject function more explicit.
As an aside, an unnecessary null check in the line below it is removed.

Test plan

Ran cadence-web locally and loaded a test workflow to ensure the output is as expected.

Before

Screenshot 2023-11-15 at 4 12 50 PM
Screenshot 2023-11-15 at 4 12 57 PM

After

Screenshot 2023-11-15 at 4 13 16 PM
Screenshot 2023-11-15 at 4 13 23 PM

To test null and undefined, I hardcoded the result:
Screenshot 2023-11-15 at 4 32 42 PM
Screenshot 2023-11-15 at 4 41 21 PM

To test the removal of the unnecessary null check, a unit test was added to ensure that the getStringElipsis util function returns the expected output.

@adhityamamallan adhityamamallan marked this pull request as ready for review November 16, 2023 09:23
@adhityamamallan adhityamamallan merged commit f64c457 into master Nov 16, 2023
4 checks passed
@adhityamamallan adhityamamallan deleted the falsy-result-fix branch November 16, 2023 10:44
This was referenced Nov 28, 2023
@adhityamamallan adhityamamallan mentioned this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants