Make Mafs line segment color offBlack64 #1073

benchristel · 2024-03-13T23:26:34Z

Summary:

We changed it (accidentally) to offBlack16 in
#1067. This commit fixes it.

Issue: none

Test plan:

View the Mafs segment graph in Storybook (http://localhost:6006/?path=/story/perseus-widgets-interactive-graph--segment-with-mafs-and-locked-points).
Verify that the segment is a darker gray than the gridlines.

We changed it (accidentally) to offBlack16 in #1067. This commit fixes it. I removed the `--mafs-line-color` custom property because it's no longer used. Issue: none Test plan: View a Mafs segment graph in the dev UI (`yarn dev`). Verify that the segment is a darker gray than the gridlines.

khan-actions-bot · 2024-03-13T23:26:50Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/two-flowers-heal.md, packages/perseus/src/widgets/interactive-graphs/mafs-styles.css

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-03-13T23:28:09Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (f4e3257) and published it to npm. You
can install it using the tag PR1073.

Example:

yarn add @khanacademy/perseus@PR1073

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1073

benchristel · 2024-03-13T23:28:14Z

I wonder if we should have visual snapshot tests to catch regressions like this 🤔

Though I don't think it would necessarily have caught this particular case—this regression happened because Nisha and I both changed the Mafs CSS concurrently in contradictory ways.

github-actions · 2024-03-13T23:29:40Z

Size Change: 0 B

Total Size: 810 kB

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.1 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.4 kB
`packages/perseus-core/dist/es/index.js`	908 B
`packages/perseus-editor/dist/es/index.js`	263 kB
`packages/perseus-error/dist/es/index.js`	878 B
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/perseus/dist/es/index.js`	385 kB
`packages/pure-markdown/dist/es/index.js`	3.69 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-03-13T23:32:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.78%. Comparing base (eb637b3) to head (f4e3257).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1073      +/-   ##
==========================================
+ Coverage   67.42%   68.78%   +1.36%     
==========================================
  Files         436      438       +2     
  Lines       96523    96568      +45     
  Branches     6749    10293    +3544     
==========================================
+ Hits        65079    66424    +1345     
+ Misses      31444    30144    -1300

see 97 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb637b3...f4e3257. Read the comment docs.

nedredmond · 2024-03-13T23:55:10Z

The screenshot is a Graphie background, not a Mafs grid

nedredmond · 2024-03-13T23:58:32Z

@benchristel there's some confusion here-- the line segment color should use the "fg" variable, or foreground, not the "line-color" variable, which is for gridlines.

Please check a Mafs grid and ensure the gridlines are the appropriate color

nedredmond

Please verify that you are not causing a regression in the Mafs grid rendering

nedredmond · 2024-03-14T00:01:46Z

packages/perseus/src/widgets/interactive-graphs/mafs-styles.css

@@ -6,8 +6,6 @@
    --mafs-bg: transparent;
    --mafs-fg: rgb(33, 36, 44); /* WB color.offBlack */

-    --mafs-line-color: rgba(33, 36, 44, 0.16); /* WB color.offBlack16*/


@nishasy and I looked at this-- this variable is used for gridlines and needs to be offBlack16 or offBlack24

@nedredmond I ended up changing it back to offBlack16, because I looked and offBlack24 is not there in WB. 24% was only added for certain colors, not black. My mistake!

nedredmond · 2024-03-14T00:02:45Z

packages/perseus/src/widgets/interactive-graphs/mafs-styles.css

@@ -23,7 +21,7 @@
 }

 .MafsView .movable-segment {
-    --mafs-segment-stroke-color: var(--mafs-line-color);


This was the wrong variable for segment stroke, should have been fg (or you can make it offBlack64 like you did)

nedredmond · 2024-03-14T00:43:23Z

I am getting the impression you mistakenly thought it was a Perseus-side custom variable. Unfortunately, Mafs doesn't have great documentation on the variables available. See its use here:

https://github.com/stevenpetryk/mafs/blob/main/src/display/Coordinates/Cartesian.tsx

Please restore it. Again, it's used to color gridlines.

This reverts commit 902fe99.

benchristel · 2024-03-14T01:16:21Z

@nedredmond Thanks, and good catch. I had a brain fart and thought --mafs-line-color was unused because searching for it showed no usages.

It's fixed now.

nedredmond · 2024-03-14T03:16:12Z

@nedredmond Thanks, and good catch. I had a brain fart and thought --mafs-line-color was unused because searching for it showed no usages.

It's fixed now.

Great! Sorry for repeating myself a bunch i reviewed on the GitHub app and couldn't see my comments

@benchristel

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus-dev-ui@1.3.0 ### Minor Changes - [#1071](#1071) [`eb637b35`](eb637b3) Thanks [@benchristel](https://github.com/benchristel)! - Add a "check answer" button to interactive graphs displayed in the dev flipbook UI ## @khanacademy/perseus@20.2.0 ### Minor Changes - [#1067](#1067) [`6196375a`](6196375) Thanks [@nishasy](https://github.com/nishasy)! - Can pass a point into `lockedFigures` into MafsGraph and a point will be displayed. - [#1074](#1074) [`a263e940`](a263e94) Thanks [@nishasy](https://github.com/nishasy)! - Add "add a locked figure" UI to interactive graph editor + adding points (mafs graphs only) ### Patch Changes - [#1068](#1068) [`881da724`](881da72) Thanks [@benchristel](https://github.com/benchristel)! - Adjust styling of the movable points and line segments on the Mafs segment graph - [#1071](#1071) [`eb637b35`](eb637b3) Thanks [@benchristel](https://github.com/benchristel)! - Add a "check answer" button to interactive graphs displayed in the dev flipbook UI - [#1073](#1073) [`ea0db7d9`](ea0db7d) Thanks [@benchristel](https://github.com/benchristel)! - Fix color of Mafs line segments ## @khanacademy/perseus-editor@5.1.0 ### Minor Changes - [#1074](#1074) [`a263e940`](a263e94) Thanks [@nishasy](https://github.com/nishasy)! - Add "add a locked figure" UI to interactive graph editor + adding points (mafs graphs only) ### Patch Changes - Updated dependencies \[[`881da724`](881da72), [`eb637b35`](eb637b3), [`6196375a`](6196375), [`a263e940`](a263e94), [`ea0db7d9`](ea0db7d)]: - @khanacademy/perseus@20.2.0 Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Extract i18n strings (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald, ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x) Pull Request URL: #1069

benchristel self-assigned this Mar 13, 2024

benchristel requested review from jeremywiebe, mark-fitzgerald, nedredmond and nishasy March 13, 2024 23:26

khan-actions-bot requested a review from a team March 13, 2024 23:26

jeremywiebe approved these changes Mar 13, 2024

View reviewed changes

Add changeset

fcdc76a

nedredmond reviewed Mar 14, 2024

View reviewed changes

benchristel added 3 commits March 13, 2024 18:06

Revert "Make Mafs line segment color offBlack64"

9645e72

This reverts commit 902fe99.

Fix Mafs segment stroke color

2fbec21

Fix lint

f4e3257

benchristel merged commit ea0db7d into main Mar 14, 2024
12 checks passed

benchristel deleted the benc/fix-segment-color branch March 14, 2024 18:40

khan-actions-bot mentioned this pull request Mar 14, 2024

Version Packages #1069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Mafs line segment color offBlack64 #1073

Make Mafs line segment color offBlack64 #1073

benchristel commented Mar 13, 2024 •

edited

Loading

khan-actions-bot commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

benchristel commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

nedredmond commented Mar 13, 2024

nedredmond commented Mar 13, 2024 •

edited

Loading

nedredmond left a comment

nedredmond Mar 14, 2024

nishasy Mar 14, 2024

nedredmond Mar 14, 2024

nedredmond commented Mar 14, 2024

benchristel commented Mar 14, 2024

nedredmond commented Mar 14, 2024

Make Mafs line segment color offBlack64 #1073

Make Mafs line segment color offBlack64 #1073

Conversation

benchristel commented Mar 13, 2024 • edited Loading

Summary:

khan-actions-bot commented Mar 13, 2024 • edited Loading

Gerald

github-actions bot commented Mar 13, 2024 • edited Loading

npm Snapshot: Published

benchristel commented Mar 13, 2024 • edited Loading

github-actions bot commented Mar 13, 2024 • edited Loading

codecov bot commented Mar 13, 2024 • edited Loading

Codecov Report

nedredmond commented Mar 13, 2024

nedredmond commented Mar 13, 2024 • edited Loading

nedredmond left a comment

Choose a reason for hiding this comment

nedredmond Mar 14, 2024

Choose a reason for hiding this comment

nishasy Mar 14, 2024

Choose a reason for hiding this comment

nedredmond Mar 14, 2024

Choose a reason for hiding this comment

nedredmond commented Mar 14, 2024

benchristel commented Mar 14, 2024

nedredmond commented Mar 14, 2024

benchristel commented Mar 13, 2024 •

edited

Loading

khan-actions-bot commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

benchristel commented Mar 13, 2024 •

edited

Loading

github-actions bot commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading

nedredmond commented Mar 13, 2024 •

edited

Loading