Skip to content
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: Remove old components and make compact components the default #2848

Merged
merged 6 commits into from
Mar 17, 2024

Conversation

CZX123
Copy link
Contributor

@CZX123 CZX123 commented Mar 17, 2024

Description

This PR refactors the components inside the CSE Machine, such that the compact components are now the default components. Fixes #2845

Changes

  • Renamed all classes and variables which mention compact.
  • Removed all old components, most notably the Grid component.
  • Removed the experimental button toggle inside the SideContentCseMachine component.

@coveralls
Copy link

coveralls commented Mar 17, 2024

Pull Request Test Coverage Report for Build 8313655941

Details

  • 142 of 165 (86.06%) changed or added relevant lines in 22 files are covered.
  • 14 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-2.2%) to 36.302%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/features/cseMachine/components/Level.tsx 18 19 94.74%
src/commons/sideContent/content/SideContentCseMachine.tsx 0 2 0.0%
src/features/cseMachine/components/arrows/Arrow.tsx 0 2 0.0%
src/features/cseMachine/components/values/FnValue.tsx 0 2 0.0%
src/features/cseMachine/CseMachineLayout.tsx 29 32 90.63%
src/features/cseMachine/components/arrows/ArrowFromControlItemComponent.tsx 0 4 0.0%
src/features/cseMachine/components/arrows/ArrowFromStashItemComponent.tsx 0 4 0.0%
src/features/cseMachine/components/arrows/ArrowFromText.tsx 10 15 66.67%
Files with Coverage Reduction New Missed Lines %
src/features/cseMachine/components/Visible.tsx 1 91.67%
src/features/cseMachine/CseMachineLayout.tsx 1 66.55%
src/features/cseMachine/components/Text.tsx 1 76.47%
src/features/cseMachine/components/values/ArrayValue.tsx 3 92.86%
src/features/cseMachine/components/values/GlobalFnValue.tsx 8 36.17%
Totals Coverage Status
Change from base Build 8312630473: -2.2%
Covered Lines: 4964
Relevant Lines: 12805

💛 - Coveralls

@RichDom2185
Copy link
Member

Is this PR ready @CZX123 ? I got the email for the review but the status is still draft.

@CZX123 CZX123 marked this pull request as ready for review March 17, 2024 04:43
@CZX123
Copy link
Contributor Author

CZX123 commented Mar 17, 2024

Yup, it's ready, was just waiting for the checks first.

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.

Are we getting rid of the old grid system with ArrayLevel, ArrowLane, FrameLevel, and Grid entirely?

Comment on lines 12 to 15
arrayUnit: ReferenceType | ReferenceType;
referencedBy: (ReferenceType | ReferenceType)[];

constructor(referencedBy: ReferenceType[]) {
constructor(referencedBy: (ReferenceType | ReferenceType)[]) {
Copy link
Member

Choose a reason for hiding this comment

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

ReferenceType | ReferenceType is just ReferenceType. Could you clean this up? Thanks!

Comment on lines 132 to -155
Layout.values.clear();
Layout.compactValues.forEach((v, d) => {
v.reset();
});
Layout.compactValues.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need the .forEach in the new Layout.values anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the loop seems to be unnecessary, I'll remove it.

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!

@martin-henz martin-henz merged commit eee9288 into master Mar 17, 2024
8 checks passed
@martin-henz martin-henz deleted the cse-component-refactor branch March 17, 2024 07:10
CYX22222003 pushed a commit that referenced this pull request Mar 22, 2024
…fault (#2848)

* Make compact components the new default and remove any mentions to the old components.
Also removes the experimental button toggle.

* Update test snapshots

* Clean up testing code a little

* Formatting changes

* Fix some minor issues
CYX22222003 pushed a commit that referenced this pull request Apr 10, 2024
…fault (#2848)

* Make compact components the new default and remove any mentions to the old components.
Also removes the experimental button toggle.

* Update test snapshots

* Clean up testing code a little

* Formatting changes

* Fix some minor issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

CSE Machine: Duplicate component definitions in components and compactComponents folders
5 participants