Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use powers of ten for large numbers #17

Merged
merged 5 commits into from
Nov 10, 2023
Merged

Use powers of ten for large numbers #17

merged 5 commits into from
Nov 10, 2023

Conversation

ericboucher
Copy link
Member

Screenshot 2023-11-08 at 7 38 37 PM

instead of exponential notation:
Screenshot 2023-11-08 at 7 18 49 PM

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.


Check the status or cancel PullRequest code review here.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 27
- 8

89% TSX
11% Jest Snapshot (tests)

Type of change

Minor Update - These changes appear to be a minor update to existing functionality and features.

Copy link

github-actions bot commented Nov 8, 2023

Build succeeded and deployed at https://bristemouth-ui-17.surge.sh
(hash 2bb40c7 deployed at 2023-11-09T13:35:06)

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

I left a suggestion for you on how to avoid injecting html in your exponent string and then having to use dangerouslySetInnerHTML to render it.

Image of Joey G Joey G


Reviewed with ❤️ by PullRequest

src/routes/Sensors/DataTable/index.tsx Outdated Show resolved Hide resolved
src/routes/Sensors/DataTable/index.tsx Outdated Show resolved Hide resolved
return (
<span>
{exponentialForm.replace(/e[+-](\d+)/, ' x 10')}
<sup>{digits}</sup>
Copy link

Choose a reason for hiding this comment

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

[nit] looks like if digits is an empty string this will leave an empty tag in the DOM. You could wrap this element in a conditional to prevent this:

{digits && <sup>{digits}</sup>}

🔹 Best Practices (Nice to have)

Image of Joey G Joey G

Copy link
Contributor

Choose a reason for hiding this comment

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

here digits should never be empty, since the regex should always match something for a number in exponential notation. Added some checks in line :)

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest reviewed the updates made to #17 up until the latest commit (2bb40c7). No further issues were found.

Reviewed by:

Image of Joey G Joey G

@ericboucher ericboucher merged commit 594f609 into main Nov 10, 2023
@ericboucher ericboucher deleted the powers-ten branch November 10, 2023 20:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants