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

Decouple Playground from SICP Workspace #2487

Draft
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

RichDom2185
Copy link
Member

Description

The tight coupling is blocking a couple planned changes related to refactoring to make use of language configs.

Fixes #2410.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Code quality improvements

How to test

Checklist

  • I have tested this code
  • I have updated the documentation

@RichDom2185 RichDom2185 requested review from ianyong and chownces May 18, 2023 04:09
@RichDom2185 RichDom2185 self-assigned this May 18, 2023
@coveralls
Copy link

coveralls commented May 18, 2023

Pull Request Test Coverage Report for Build 5010569236

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 237 (0.84%) changed or added relevant lines in 2 files are covered.
  • 68 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 34.197%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/pages/playground/Playground.tsx 0 1 0.0%
src/pages/sicp/subcomponents/SicpWorkspace.tsx 2 236 0.85%
Files with Coverage Reduction New Missed Lines %
src/pages/playground/Playground.tsx 68 1.94%
Totals Coverage Status
Change from base Build 5010320171: -0.6%
Covered Lines: 4916
Relevant Lines: 13400

💛 - Coveralls

Replaces isSicpEditor with the literal `true` and simplify the
conditionals that involve it. This results in removal of features such
as the remote execution tab and the shared session buttons.
Replaces isSicpEditor with the literal `false` and simplify the
conditionals that involve it.
Removes from SicpWorkspace remaining query string handling logic that
was only used for remote execution (`shouldAddDevice` param).
@RichDom2185 RichDom2185 added the blocked Something else needs pass review first label May 18, 2023
@RichDom2185
Copy link
Member Author

This PR will first require the language config object to be moved from a hardcoded Playground state to be under the workspace context instead. Currently, the SICP Workspace still inherits and affects the state of the Playground, so this should not be merged yet.

@ianyong
Copy link
Member

ianyong commented May 24, 2023

This PR will first require the language config object to be moved from a hardcoded Playground state to be under the workspace context instead. Currently, the SICP Workspace still inherits and affects the state of the Playground, so this should not be merged yet.

The overall direction of this PR looks good. I'll look through the final version of this PR thoroughly since it's mostly checking that the workspaces are separated correctly.

Partially reverts 2243494 in
preparation for merging of commits from refactoring change. Done simply
to prevent as many merge conflicts as possible. Changes are to be
restored later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something else needs pass review first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple playground & SICP workspaces
3 participants