Skip to content

Conditional tab rendering in Playground #808

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.

Sign up for GitHub

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
martin-henz merged 11 commits into from
Aug 11, 2019
Merged

Conditional tab rendering in Playground #808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0bfad59
Conditional tab rendering for Playground
alcen Aug 9, 2019
ad8c3d9
Fix For Crashing in Source §1 & §2
lumos309 Aug 6, 2019
88d57ae
Merge branch 'master' of https://github.com/source-academy/cadet-fron…
alcen Aug 9, 2019
5f2dc6e
Merge branch 'master' into conditional-tabs
alcen Aug 9, 2019
928acd9
Merge branch 'master' into conditional-tabs
alcen Aug 9, 2019
9e7a7f0
Merge branch 'master' into conditional-tabs
geshuming Aug 9, 2019
26ba009
Merge branch 'master' into conditional-tabs
martin-henz Aug 9, 2019
f5d4af6
Merge branch 'master' into conditional-tabs
alcen Aug 11, 2019
1561eec
Merge branch 'master' into conditional-tabs
alcen Aug 11, 2019
85c41c8
Merge branch 'master' into conditional-tabs
martin-henz Aug 11, 2019
b806b10
Merge branch 'master' into conditional-tabs
martin-henz Aug 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/components/Playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,19 +192,25 @@ class Playground extends React.Component<IPlaygroundProps, PlaygroundState> {
/>
);

const tabs: SideContentTab[] = [
playgroundIntroductionTab,
listVisualizerTab,
inspectorTab,
envVisualizerTab
];
const tabs: SideContentTab[] = [playgroundIntroductionTab];

// Conditional logic for tab rendering
if (
this.props.externalLibraryName === ExternalLibraryNames.PIXNFLIX ||
this.props.externalLibraryName === ExternalLibraryNames.ALL
) {
// Enable video tab only when 'PIX&FLIX' is selected
tabs.push(videoDisplayTab);
}
if (this.props.sourceChapter >= 2) {
// Enable Data Visualizer for Source Chapter 2 and above
tabs.push(listVisualizerTab);
}
if (this.props.sourceChapter >= 3) {
// Enable Inspector, Env Visualizer for Source Chapter 3 and above
tabs.push(inspectorTab);
tabs.push(envVisualizerTab);
}

const workspaceProps: WorkspaceProps = {
controlBarProps: {
Expand Down
2 changes: 1 addition & 1 deletion src/sagas/workspaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ export function* evalCode(
context.runtime.debuggerOn =
(actionType === actionTypes.EVAL_EDITOR || actionType === actionTypes.DEBUG_RESUME) &&
context.chapter > 2;
if (!context.runtime.debuggerOn) {
if (!context.runtime.debuggerOn && context.chapter > 2) {
Copy link
Contributor

@geshuming geshuming Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can we make this more logical, this part has always been making my brain explode.

const useDebugger = actionType !== actionTypes.EVAL_EDITOR && actionType !== actionTypes.DEBUG_RESUME && context.chapter > 2;

if (useDebugger) {
  inspectorUpdate(undefined);
}

Honestly though, I don't even think the above code segment makes any sense. Please comment on what this code segment is even intended to do.

Copy link
Contributor

@geshuming geshuming Aug 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, debuggerOn is used as a flag for slang.

So the correct code should look like

context.runtime.debuggerOn = (actionType === actionTypes.EVAL_EDITOR || actionType === actionTypes.DEBUG_RESUME) && context.chapter > 2;

if (context.runtime.debuggerOn) {
  // Resets the inspector to prepare for new output
  inspectorUpdate(undefined);
}

Copy link
Contributor

@lumos309 lumos309 Aug 8, 2019

Choose a reason for hiding this comment

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

This was done by the INSPECT team, so I'm not certain about how it works either. Having said that, this:

Okay, debuggerOn is used as a flag for slang.

So the correct code should look like

context.runtime.debuggerOn = (actionType === actionTypes.EVAL_EDITOR || actionType === actionTypes.DEBUG_RESUME) && context.chapter > 2;

if (context.runtime.debuggerOn) {
  // Resets the inspector to prepare for new output
  inspectorUpdate(undefined);
}

doesn't seem to be the same boolean expression as the original code. !(actionType === actionTypes.EVAL_EDITOR || actionType === actionTypes.DEBUG_RESUME) && context.chapter > 2 will call inspectorUpdate(undefined) in the original code, but not in the suggested amendment.

Copy link
Contributor

@geshuming geshuming Aug 8, 2019

Choose a reason for hiding this comment

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

My mistake. The clause should be

if (!context.runtime.debuggerOn)

just like the original code

Copy link
Contributor

@lumos309 lumos309 Aug 8, 2019

Choose a reason for hiding this comment

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

No, this won't work because it can trigger inspectedUpdate(undefined) even when !(context.chapter > 2), and that crashes the entire webpage. That's what the change was originally for.

Copy link
Contributor

@geshuming geshuming Aug 8, 2019

Choose a reason for hiding this comment

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

Ok, can we clean this up then. The logic here is very confusing. Can we add comments to clarify?

inspectorUpdate(undefined); // effectively resets the interface
}
const { result, interrupted, paused } = yield race({
Expand Down