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.

Sign up for GitHub

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

Jump to bottom

replace toFixed #18

Closed
pixelzoom opened this issue Feb 8, 2020 · 2 comments
Closed

replace toFixed #18

pixelzoom opened this issue Feb 8, 2020 · 2 comments
Labels
dev:code-review

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 8, 2020 •
edited by Hyodar
Loading

Related to #2 (code review).

  • DOT/Utils.toFixed or DOT/Utils.toFixedNumber should be used instead of toFixed. JavaScript's toFixed is notoriously buggy. Behavior differs depending on browser, because the spec doesn't specify whether to round or floor.

The sim uses toFixed where it should use DOT/Utils toFixed or toFixedNumber. See 2 occurrences in AmpPhaseAccordionBox.js.
@pixelzoom pixelzoom mentioned this issue Feb 8, 2020
Closed
Hyodar added a commit that referenced this issue Feb 12, 2020
@Hyodar
replace toFixed, #18
97a01aa
@Hyodar
Copy link
Contributor

Hyodar commented Feb 12, 2020

Replaced toFixed with Utils.toFixed. There was no need to return it as a number, so Utils.toFixed was fine.

@Hyodar
Copy link
Contributor

Hyodar commented Feb 17, 2020

About these strings, there's still #12 to solve, but the toFixed usage now seems fine, so I'll go ahead and close this issue.

@Hyodar Hyodar closed this as completed Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dev:code-review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pixelzoom @Hyodar