-
Notifications
You must be signed in to change notification settings - Fork 171
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
CSE Machine: Added animation for spread instruction #3114
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 14290737373Details
💛 - Coveralls |
Looking at it, might take a while though since I'm busy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please remove the print statements.
src/features/cseMachine/animationComponents/ArraySpreadAnimation.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good if you have a screen recording showing the new animation in action!
craco.config.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert the formatting changes to this file (and just this file) too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay done, but now for some reason the lint coveralls failed Error: Bad response: 405 . Not sure why this is happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably their service is down, just wait a while
@@ -28,6 +28,7 @@ export class ArrayAccessAnimation extends Animatable { | |||
private resultAnimation: AnimatedTextbox; | |||
private resultArrowAnimation?: AnimatedGenericArrow<StashItemComponent, Visible>; | |||
private arrayUnit: ArrayUnit; | |||
private out: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does out
mean? Can clarify? From the name, it's not immediately clear what this does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to outOfRange. if the array access is our of range, outOfRange is false, true otherwise
Description
Partially deals with #3110, further work can be done on array access out of bounds
Type of change
20250312-1921-34.2597022.mp4
Checklist