Skip to content

Commit

Permalink
BUGFIX - Leftmost number in Number Line widget misaligned (#1695)
Browse files Browse the repository at this point in the history
## Summary:
The Number Line widget contains display logic that adds duplicate endpoint numbers, and then colors them blue if the widget is being used on mobile. This duplicate endpoint becomes misaligned when styling is accounting for the reduced viewport size. This bugfix corrects that misalignment, and removes the mobile logic in favor of standardizing across all platforms (both mobile and desktop now have blue endpoints).

Issue: LEMS-2383

## Test plan:

1. Launch Storybook
1. Review the [Number Line](http://localhost:6006/?path=/story/perseus-widgets-number-line--question-1) widget
   - Both endpoints should be blue
   - All numbers on the line should be positioned the same vertically

## Affected behavior:
### Before
![Number Line Endpoint - Before](https://github.com/user-attachments/assets/951a3c2b-062f-4204-8744-03931a7c5ab4)

### After
![Number Line Endpoint - After](https://github.com/user-attachments/assets/5fcef568-85ce-4634-b862-aac9237811df)

Author: mark-fitzgerald

Reviewers: catandthemachines, mark-fitzgerald, #perseus

Required Reviewers:

Approved By: catandthemachines

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1695
  • Loading branch information
mark-fitzgerald authored Oct 8, 2024
1 parent 92c4e62 commit 387273b
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 64 deletions.
5 changes: 5 additions & 0 deletions .changeset/nasty-bikes-dress.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

BUGFIX: Left-most digit in Number Line widget is misaligned
13 changes: 13 additions & 0 deletions packages/perseus/src/util/graphie.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1675,6 +1675,8 @@ const setLabelMargins = function (span: HTMLElement, size: Coord): void {
marginTop: -height / 2 - y * scale,
});
} else {
const currentHeightMatchesProps = span.scrollHeight === height;

// We are using jQuery to collect information and calculate a scale
// since we don't have a way to pass it to this function.
// We need the width of the container in order to calculate the scale to apply to the label.
Expand All @@ -1696,6 +1698,17 @@ const setLabelMargins = function (span: HTMLElement, size: Coord): void {
// Inherited line-height values can really mess up placement.
$container.css("line-height", "normal");

// TODO: Verify that this change doesn't impact other graphies
// Check the full list of consuming widgets.

// If the change in line-height affected the height of the element,
// then the height used for calculations should be updated.
// This can happen when the first label in the container calls this method,
// and the line-height was different when the height measurement was originally referenced.
if (currentHeightMatchesProps && span.scrollHeight !== height) {
height = span.scrollHeight;
}

// The expected width of the graphie is found in the "max-width" property on ".svg-image" containers,
// and is found in the "width" property on ".graphie" containers.
const widthValues = $container.css(["max-width", "width"]) ?? {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ exports[`number-line widget should snapshot on mobile: first mobile render 1`] =
<span
class="graphie-label"
data-math-formula="-4"
style="position: absolute; padding: 7px; color: black; left: 29.999999999999993px; top: 61.2px; fill: none;"
style="position: absolute; padding: 7px; color: rgb(24, 101, 242); left: 29.999999999999993px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
Expand Down Expand Up @@ -372,24 +372,6 @@ exports[`number-line widget should snapshot on mobile: first mobile render 1`] =
class="tex-holder"
/>
</span>
<span
class="graphie-label"
data-math-formula="4"
style="position: absolute; padding: 7px; color: black; left: 258px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
/>
</span>
<span
class="graphie-label"
data-math-formula="-4"
style="position: absolute; padding: 7px; color: rgb(24, 101, 242); left: 29.999999999999993px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
/>
</span>
<span
class="graphie-label"
data-math-formula="4"
Expand Down Expand Up @@ -707,7 +689,7 @@ exports[`number-line widget should snapshot: first render 1`] = `
<span
class="graphie-label"
data-math-formula="-4"
style="position: absolute; padding: 7px; color: black; left: 29.999999999999982px; top: 61.2px; fill: none;"
style="position: absolute; padding: 7px; color: rgb(24, 101, 242); left: 29.999999999999982px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
Expand Down Expand Up @@ -779,25 +761,7 @@ exports[`number-line widget should snapshot: first render 1`] = `
<span
class="graphie-label"
data-math-formula="4"
style="position: absolute; padding: 7px; color: black; left: 430px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
/>
</span>
<span
class="graphie-label"
data-math-formula="-4"
style="position: absolute; padding: 7px; color: black; left: 29.999999999999982px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
/>
</span>
<span
class="graphie-label"
data-math-formula="4"
style="position: absolute; padding: 7px; color: black; left: 430px; top: 61.2px; fill: none;"
style="position: absolute; padding: 7px; color: rgb(24, 101, 242); left: 430px; top: 61.2px; fill: none;"
>
<span
class="tex-holder"
Expand Down
34 changes: 9 additions & 25 deletions packages/perseus/src/widgets/number-line/number-line.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -160,34 +160,18 @@ const TickMarks: any = Graphie.createSimpleClass((graphie, props) => {

const labelTicks = props.labelTicks;
if (labelTicks || props.labelStyle === "decimal ticks") {
results.push(_label(graphie, props.labelStyle, x, x, base));
if (x === leftLabel || x === rightLabel) {
results.push(
graphie.style({color: KhanColors.BLUE}, () =>
_label(graphie, props.labelStyle, x, x, base),
),
);
} else {
results.push(_label(graphie, props.labelStyle, x, x, base));
}
}
}

// Render the text labels
results.push(
graphie.style(
props.isMobile
? {
color: KhanColors.BLUE,
}
: {},
() => _label(graphie, props.labelStyle, leftLabel, leftLabel, base),
),
);

results.push(
graphie.style(
props.isMobile
? {
color: KhanColors.BLUE,
}
: {},
() =>
_label(graphie, props.labelStyle, rightLabel, rightLabel, base),
),
);

// Render the labels' lines
graphie.style(
{
Expand Down

0 comments on commit 387273b

Please sign in to comment.