Fixed the significand to have only one integer digit#408
Fixed the significand to have only one integer digit#408brunescot wants to merge 1 commit intofalconry:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates how the site’s “lifetime” value is displayed, aiming to format it using scientific notation rather than hardcoding an exponent in the HTML.
Changes:
- Updated the homepage tagline to remove the hardcoded
× 10⁻²factor. - Added a
convSciNotation()formatter and wired it into the lifetime ticker display logic. - Added a new npm dependency (
copy-webpack) and updated the lockfile accordingly.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/index.html |
Removes the hardcoded scientific-notation factor from the lifetime tagline. |
src/dynamics.js |
Adds scientific-notation formatting and applies it to the lifetime animation. |
package.json |
Adds copy-webpack to dependencies. |
package-lock.json |
Lockfile updates reflecting copy-webpack and dependency classification changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function convSciNotation(qty) { | ||
| const exponent = Math.floor(Math.log10(qty)); | ||
| const mantissa = qty / Math.pow(10, exponent); | ||
| return `${mantissa.toFixed(3)} x 10 <sup> ${(-1) * exponent}</sup>`; |
There was a problem hiding this comment.
convSciNotation() is negating the exponent in the rendered string. Given exponent = floor(log10(qty)) and mantissa = qty / 10^exponent, the correct scientific-notation representation is mantissa × 10^exponent; using -exponent will produce the reciprocal power (and is only correct for exponent === 0). Render the original exponent (including the sign) instead of negating it.
| return `${mantissa.toFixed(3)} x 10 <sup> ${(-1) * exponent}</sup>`; | |
| return `${mantissa.toFixed(3)} x 10 <sup> ${exponent}</sup>`; |
| function convSciNotation(qty) { | ||
| const exponent = Math.floor(Math.log10(qty)); | ||
| const mantissa = qty / Math.pow(10, exponent); | ||
| return `${mantissa.toFixed(3)} x 10 <sup> ${(-1) * exponent}</sup>`; |
There was a problem hiding this comment.
The comment says "3 significant digits", but mantissa.toFixed(3) keeps 3 digits after the decimal, yielding 4 significant digits for a 1–9 mantissa (e.g., 1.234). If you want 3 significant digits, use a significant-figures approach (e.g., toPrecision(3) on the mantissa) or adjust rounding logic accordingly.
| return `${mantissa.toFixed(3)} x 10 <sup> ${(-1) * exponent}</sup>`; | |
| return `${mantissa.toPrecision(3)} x 10 <sup> ${(-1) * exponent}</sup>`; |
| let tempLifetime = convSciNotation(lifetime); | ||
|
|
||
| function updateLifetime() { | ||
| lifetimeTick += lifetime / 50; | ||
| if (lifetimeTick < lifetime) { | ||
| lifetimeElement.innerHTML = String(lifetimeTick.toFixed(2)); | ||
| lifetimeElement.innerHTML = String(convSciNotation(lifetimeTick.toFixed(2))); | ||
| if ((lifetimeTick / lifetime) > 0.8) { |
There was a problem hiding this comment.
lifetime and lifetimeTick are being passed through toFixed() before conversion, which turns them into strings and introduces double-rounding. This relies on implicit numeric coercion in Math.log10()/division and can produce inconsistent scientific notation as the value grows. Keep the values as numbers (parse once), and only format/round at the final display step inside convSciNotation().
| let tempLifetime = convSciNotation(lifetime); | ||
|
|
||
| function updateLifetime() { | ||
| lifetimeTick += lifetime / 50; | ||
| if (lifetimeTick < lifetime) { | ||
| lifetimeElement.innerHTML = String(lifetimeTick.toFixed(2)); | ||
| lifetimeElement.innerHTML = String(convSciNotation(lifetimeTick.toFixed(2))); | ||
| if ((lifetimeTick / lifetime) > 0.8) { | ||
| lifetimeDelay *= 1.1; | ||
| } | ||
| setTimeout(updateLifetime, lifetimeDelay); | ||
| } | ||
| else { | ||
| lifetimeElement.innerHTML = lifetime; | ||
| lifetimeElement.innerHTML = tempLifetime; | ||
| } |
There was a problem hiding this comment.
The UI text now says "centuries" without the previous × 10⁻² scaling, but lifetime is still calculated as years (deltaSec / SECONDS_PER_ORBITAL_ROTATION). This will display the wrong unit unless you convert years → centuries (divide by 100) before formatting, or change the label back to years.
| <div id="mainTitle" class="container text-center mt-6"> | ||
| <h1 class="block-title display-1">Falcon</h1> | ||
| <p class="text-muted">Unburdening APIs for over <span id="lifetime">0.00</span> x 10<sup>-2</sup> centuries.</p> | ||
| <p class="text-muted">Unburdening APIs for over <span id="lifetime">0.00</span> centuries.</p> |
There was a problem hiding this comment.
This copy change removes the previous × 10⁻² factor that implicitly converted the displayed value from years to centuries, but the JS still computes lifetime in years. Either convert the underlying value to centuries before rendering, or update the text to say "years" to keep units consistent.
| <p class="text-muted">Unburdening APIs for over <span id="lifetime">0.00</span> centuries.</p> | |
| <p class="text-muted">Unburdening APIs for over <span id="lifetime">0.00</span> years.</p> |
| "@popperjs/core": "^2.11.8", | ||
| "bootstrap": "^5.3.8", | ||
| "bootstrap-icons": "^1.13.1", | ||
| "copy-webpack": "^5.0.4", |
There was a problem hiding this comment.
copy-webpack is added as a runtime dependency, but the build already uses copy-webpack-plugin directly (see webpack configs) and there are no references to copy-webpack in the repo. This extra dependency also pulls in a nested copy-webpack-plugin@12 and increases install surface. Remove copy-webpack (or, if it’s actually needed for the build, move it to devDependencies and update the configs to use it explicitly).
| "copy-webpack": "^5.0.4", |
| // function to convert a quantity to scientific notation with 3 significant digits | ||
| function convSciNotation(qty) { | ||
| const exponent = Math.floor(Math.log10(qty)); | ||
| const mantissa = qty / Math.pow(10, exponent); | ||
| return `${mantissa.toFixed(3)} x 10 <sup> ${(-1) * exponent}</sup>`; | ||
| } |
There was a problem hiding this comment.
PR description says the formatting function was created in index.js, but the new convSciNotation() helper is defined in src/dynamics.js. Update the PR description (or move the function) so the documentation matches the actual change.
I created a function in index.js to return a
spanwith the desired format.The number of significant digits can be altered in the function (`.toFixed(n)