Fix flaky TeX Chromatic snapshots for Matcher and Sorter widgets#3692
Fix flaky TeX Chromatic snapshots for Matcher and Sorter widgets#3692Myranae wants to merge 11 commits into
Conversation
…romatic snapshots for Matcher and Sorter widgets.
…ithAssetContext wrapper
…r-sorter-asset-tracking
|
Size Change: +340 B (+0.07%) Total Size: 508 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (2be2a63) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3692If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3692If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3692 |
|
@claude review once |
|
@benchristel After your review on the previous iteration of this work, I decided to start a new PR with the new direction. I went over this pretty closely with Claude, but still waiting for the Github Claude review to finish and will get to that on Monday. Does this new direction align more with how you would expect this to look? It no longer uses any sort of timer mechanic. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
jeremywiebe
left a comment
jeremywiebe
left a comment
There was a problem hiding this comment.
I think this looks right. I'll hold off approving as Ben had the original thoughts on this re-working.
| return ( | ||
| <button | ||
| onClick={() => props.setAssetStatus(props.assetKey, true)} | ||
| data-testid={props.label} |
There was a problem hiding this comment.
nit: If you omit this, can you use screen.getByRole("button", {name: /label/}); instead? That'd avoid using test ids, I think.
| ); | ||
| }); | ||
|
|
||
| WithAssetContextComponent.displayName = `withAssetContext(${ |
There was a problem hiding this comment.
Nice. This is helpful!
| const currentConstraint = _.max([ | ||
| this.state.leftHeight, | ||
| this.state.rightHeight, | ||
| ]); |
There was a problem hiding this comment.
Can we use Math.max() instead?
| const currentConstraint = _.max([ | |
| this.state.leftHeight, | |
| this.state.rightHeight, | |
| ]); | |
| const currentConstraint = Math.max( | |
| this.state.leftHeight, | |
| this.state.rightHeight, | |
| ); |
| const currentConstraint = _.max([ | ||
| this.state.leftHeight, | ||
| this.state.rightHeight, | ||
| ]); |
There was a problem hiding this comment.
Can we use Math.max() instead?
| const currentConstraint = _.max([ | |
| this.state.leftHeight, | |
| this.state.rightHeight, | |
| ]); | |
| const currentConstraint = Math.max( | |
| this.state.leftHeight, | |
| this.state.rightHeight, | |
| ); |
Myranae
changed the title
Fix some decorator issues and also switch TeX stories to serverItemRenderer so they actually connect to the AssetContext.Provider
…c column heights onMeasureLeft/Right use`height === currentConstraint` to decide a side had stabilized, but onMeasure receives natural (unconstrained) heights. The shorter column's natural height never equals the constraint set by the taller column, so _leftStable stayed false forever and _maybeSettle never fired. The correct condition is `height <= currentConstraint`: a side won't push the constraint higher — and therefore won't trigger another re-measurement cycle — as long as its natural height is at or below the current constraint.
2 participants
Summary:
ServerItemRenderer.onRenderedfired as soon as TeX finished typesetting, but before the cross-column measurement cascade settled — so snapshots could capture intermediate layouts.withAssetContext, a small HOC that mirrorswithDependenciesand injectssetAssetStatusplus auseId()-generatedassetKeyas props. This lets class-component widgets consume AssetContext without changing their existingcontextType.Issue: LEMS-4117
Test plan:
Widgets/Matcher/Visual Regression Tests/Initial State/With TeXrenders stably with no layout flicker.Widgets/Sorter/Visual Regression Tests/Initial State/With TeXrenders stably.WithTeXstories produce stable baselines across multiple runs.pnpm test,pnpm tsc,pnpm lint.