Skip to content

CSE Machine: Fix some UI & animations issues, and add tests #2936

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.

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

Merged
merged 39 commits into from
Aug 10, 2025

Conversation

CZX123
Copy link
Contributor

@CZX123 CZX123 commented Apr 14, 2024

Description

This PR fixes some UI & animations issues, and add some tests for the CSE machine's layout and animations.
Fixes #2921 fully by correctly calculating the width of the frame with a rune.
Also see: #2959.
Fixes #2716. The new result is shown below:

Changes

UI

  • Improved environment frame positioning, spacing is now more consistent.
  • Global frame default text of :::pre-declared names::: now should appear first, above all bindings
  • Arrows from arrays now always exit the array first before pointing to their targets.
    • If the arrow comes from the first unit of the array to itself, it will now form a circular loop
  • Changed height and width behavior for array and function values
    • _height and _width now only refer to the height and widths of the array boxes or closure circles only
    • ArrayValue now has a totalHeight and totalWidth property that stores the total height and width of nested values inside the array
    • FnValue and GlobalFnValue has a totalWidth property that also includes the tooltip width
  • Double quotes are used for strings in the stash now.

Animations

  • Fixed frame arrows not animating alongside the frame creation in FrameCreationAnimation
  • Fixed the animation function inside BaseAnimationComponent, so that the example below has the correct expected behavior:
    a.animateTo({ x: 100 });
    a.animateTo({ x: 200 }, { delay: 1 });

Testing

  • Add test cases for the CSE machine's layout for values
  • Add test cases for the CSE machine's AnimationComponent

@coveralls
Copy link

coveralls commented Apr 14, 2024

Pull Request Test Coverage Report for Build 16860915421

Details

  • 151 of 197 (76.65%) changed or added relevant lines in 17 files are covered.
  • 12 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.2%) to 43.458%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/features/cseMachine/components/ControlItemComponent.tsx 1 2 50.0%
src/features/cseMachine/components/StashItemComponent.tsx 1 2 50.0%
src/features/cseMachine/components/Frame.tsx 39 43 90.7%
src/features/cseMachine/animationComponents/FrameCreationAnimation.tsx 1 8 12.5%
src/features/cseMachine/CseMachineLayout.tsx 12 28 42.86%
src/features/cseMachine/CseMachineAnimation.tsx 2 19 10.53%
Files with Coverage Reduction New Missed Lines %
src/features/cseMachine/animationComponents/FrameCreationAnimation.tsx 1 18.68%
src/features/cseMachine/CseMachineUtils.ts 1 71.65%
src/features/cseMachine/animationComponents/base/AnimationComponents.tsx 4 75.65%
src/features/cseMachine/components/arrows/ArrowFromText.tsx 6 81.82%
Totals Coverage Status
Change from base Build 16858234296: 0.2%
Covered Lines: 20627
Relevant Lines: 49670

💛 - Coveralls

@CZX123 CZX123 self-assigned this Apr 16, 2024
@martin-henz martin-henz marked this pull request as ready for review June 11, 2025 01:21
@martin-henz martin-henz marked this pull request as draft June 11, 2025 01:21
@RichDom2185 RichDom2185 marked this pull request as ready for review June 27, 2025 18:51
@RichDom2185 RichDom2185 enabled auto-merge (squash) June 27, 2025 18:52
@RichDom2185 RichDom2185 requested review from sayomaki and removed request for alsonleej and chuckyang123 June 27, 2025 18:53
@RichDom2185
Copy link
Member

@sayomaki do you have time to review this? It's a pure FE change and the PR seems ready, I don't see a reason not to merge it if it's all good

@martin-henz
Copy link
Member

Update: to be merged by 29/9/2025, to be ready for Unit 3 of CS1101S.

@RichDom2185 RichDom2185 disabled auto-merge August 10, 2025 11:02
@RichDom2185 RichDom2185 requested review from RichDom2185 and removed request for sayomaki August 10, 2025 11:47
@RichDom2185
Copy link
Member

Fixes #2959.

Still getting the issue with the sample program provided in the issue (+ other programs)

image image

I'll remove this from the issues closed by this PR. But feel free to close if I'm mistaken.

Fixes #2716.

Can't test right now due to issues with immer but I'll take your word for it.

@RichDom2185 RichDom2185 enabled auto-merge (squash) August 10, 2025 11:54
@RichDom2185 RichDom2185 disabled auto-merge August 10, 2025 11:55
@RichDom2185 RichDom2185 enabled auto-merge (squash) August 10, 2025 11:55
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Just the above comment

@RichDom2185 RichDom2185 merged commit 72a7e51 into master Aug 10, 2025
9 checks passed
@RichDom2185 RichDom2185 deleted the cse-uiux3 branch August 10, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

CSE: Imported rune constants appear to be bound to strings; formating issues CSE Machine: Overlapping drawing of pairs
4 participants