feature(website): Toggle Tree Style #2563

NicholasLYang · 2022-05-09T17:46:23Z

Summary

As some have pointed out, the JSON view is not optimal for some situations. This allows users to toggle between the text and JSON view.

Test Plan

cloudflare-workers-and-pages · 2022-05-09T17:48:31Z

Deploying with Cloudflare Pages

Latest commit:	`bd71c68`
Status:	✅ Deploy successful!
Preview URL:	https://63ecfd24.tools-8rn.pages.dev

View logs

github-actions · 2022-05-09T18:17:25Z

Playground for commit fef8115

website/playground/src/TreeStyleSelect.tsx

IWANABETHATGUY · 2022-05-10T02:44:18Z

Could you add some screenshots or videos about the change of this pr?

IWANABETHATGUY · 2022-05-10T03:40:46Z

Could you add some screenshots or videos about the change of this pr?

Sorry, I didn't see the preview playground

MichaReiser · 2022-05-10T06:03:44Z

Could you add some screenshots or videos about the change of this pr?

Sorry, I didn't see the preview playground

I would still find it useful to have a proper test summary because it safes me time as a reviewer:

No need to open the playground and go searching for the new option
It helps me understand how you tested your change. Thish enables me to make suggestion in case I'm aware of something that needs testing too.
It acts as documentation for the future. Assume Rome continues to grow and some PR that you haven't been aware of breaks one of your features. Proper test cases are useful in this scenario because it makes it easier for you do understand what changed and indeed, check if what you're looking into was tested or should have been tested.

MichaReiser

The label seems to be missing on mobile

MichaReiser · 2022-05-10T06:04:58Z

website/playground/src/DesktopPlayground.tsx

-							<TreeView tree={cst} />
+							<TreeView
+								treeStyle={treeStyle}
+								setTreeStyle={setTreeStyle}


Events are commonly called onTreeStyleChanged to align it with HTML event handlers.

Ehh I'd actually push back on this. I don't think it's more declarative to use the on-prefix, as these functions aren't running when the tree style changes, they're making the tree style change.

The main reason why I would change it is because TreeView now makes very specific assumption in what context its used. The signature implies that setTreeStyle must set the tree style. It kind of takes the choice from the parent what it want's to do in that case.

I'm confused, how does TreeView make an assumption about the context in which it's used?

The name setTreeStyle implies that the using component has to set the treeStyle just by how it's named. onTreeStyleChanged on the other hand doesn't make any implication what the parent does in that case. It's like... here, this thing happened... do what ever you like, I don't care. Whereas setTreeStyle reads more like... Hey, set the tree state.

website/playground/src/TreeStyleSelect.tsx

website/playground/src/TreeView.tsx

MichaReiser · 2022-05-10T06:08:02Z

website/playground/src/types.ts

@@ -21,6 +22,8 @@ export interface PlaygroundState {
 	setIsTypeScript: (isTypeScript: boolean) => void;
 	isJsx: boolean;
 	setIsJsx: (isJsx: boolean) => void;
+	treeStyle: TreeStyle;
+	setTreeStyle: Dispatch<SetStateAction<TreeStyle>>;


Nit: Change to plain function

MichaReiser · 2022-05-10T06:11:16Z

website/playground/src/utils.ts

@@ -98,6 +103,8 @@ export function usePlaygroundState(): PlaygroundState {
 		setIsJsx,
 		sourceType,
 		setSourceType,
+		treeStyle,
+		setTreeStyle,


This is becoming hard to navigate. Separating this into a single state object with a reducer that allows changing single fields would remove the fields by about 50%.

Another alternative is to store the values in a context and having a provider + consumer hook.

Yeah..I was considering gasp adding Redux because we're reaching the point where we have enough state such that Redux might be worth it

I don't think you need to add Redux just for that. A simple useContext should do to have shared state or change it to useDispatcher.

NicholasLYang · 2022-05-10T16:33:58Z

website/playground/src/SettingsMenu.tsx

-interface Props { settings: PlaygroundSettings }
+interface Props {
+	settings: PlaygroundSettings;
+	setPlaygroundState: Dispatch<SetStateAction<PlaygroundState>>;


@MichaReiser I'm using the complicated type signature here because I'm making use of the setPlaygroundState(state => ...) pattern

MichaReiser · 2022-05-11T10:01:37Z

website/playground/src/utils.ts

-		(searchParams.get("sourceType") as SourceType) ?? SourceType.Module,
-	);
-	const [treeStyle, setTreeStyle] = useState(TreeStyle.Json);
+	const [playgroundState, setPlaygroundState] = useState({


You may want to move this into a lambda so that the initialize code only runs for the first render:

const [..] = useState(() => ({....}))

Ah yeah my bad, forgot to do that

MichaReiser · 2022-05-11T10:09:40Z

website/playground/src/utils.ts

+export function createSetter(
+	setPlaygroundState: Dispatch<SetStateAction<PlaygroundState>>,
+	field: keyof PlaygroundState,
+) {
+	return function (param: PlaygroundState[typeof field]) {
+		setPlaygroundState((state) => ({ ...state, [field]: param }));


I think what you're trying to replicate is useReducer

The idea is to have one action for each field.

Overall, what you're trying to do here feels complicated with passing down the setPlaygroundState and then having a createSetter. For someone to understand the code they must first understand what setPlaygroundState is, then what createSetter does` for something that's otherwise relatively simple.

I think what you want here is useContext which helps you avoid the prop drilling (passing down many props multiple level-deep).

The provider doesn't need to accept any props because it initializes the state from the URL params.

They consumer can then expose the state + a method to change a single field

const [{ code }, dispatchPlaygroundAction] = usePlaygroundState(); dispatchPlaygroundAction("change_code", code);

Ehhh. I just did the useReducer refactor with useContext. It gets very hairy. First, useContext is quite annoying in that you can't call createContext inside a component, so it doesn't play well with other hooks like useReducer. You have to initialize the context values to undefined/null, or initialize them in the context, but then re-initialize them in the reducer as well. If you initialize them to undefined/null, TypeScript gets very annoyed (understandably) about you using potentially undefined variables.

With actions, they are more explicit, but I could also easily make this more explicit by not using createSetter and just using setter functions explicitly. In my experience useReducer makes more sense when you need to do actions more complicated than set, i.e. actions where you change multiple parts of the state at once and need the abstraction of an action over just a setter. In this case while we have a lot of state, we're almost always just setting one field.

Having done this refactor just now, I have to say, it makes me a little more pro-Redux. With the starter kit, Redux is not much more overhead than useReducer + useContext and it avoids all of these nasty undefined/null issues.

MichaReiser · 2022-05-11T11:52:52Z

We can do the refactor changes as part of another PR. Please update your test plan with a screenshot for Mobile/Desktop.

NicholasLYang requested a review from MichaReiser May 9, 2022 17:46

NicholasLYang temporarily deployed to aws May 9, 2022 17:46 Inactive

NicholasLYang temporarily deployed to aws May 9, 2022 18:12 Inactive

mrkldshv reviewed May 9, 2022

View reviewed changes

website/playground/src/TreeStyleSelect.tsx Outdated Show resolved Hide resolved

website/playground/src/TreeStyleSelect.tsx Outdated Show resolved Hide resolved

NicholasLYang temporarily deployed to aws May 9, 2022 19:55 Inactive

MichaReiser suggested changes May 10, 2022

View reviewed changes

NicholasLYang temporarily deployed to aws May 10, 2022 15:32 Inactive

NicholasLYang force-pushed the feature/toggle_tree_view branch from 859124c to cde64e2 Compare May 10, 2022 15:54

NicholasLYang commented May 10, 2022

View reviewed changes

MichaReiser reviewed May 11, 2022

View reviewed changes

MichaReiser approved these changes May 11, 2022

View reviewed changes

Nicholas Yang added 10 commits May 11, 2022 17:21

Added select for tree style

e8479e6

Formatting

ef0d1c8

Toggling tree view for json or text

23c8a4d

Removed unused dependencies

ef8b779

Added treeStyle to mobile playground

62cc889

PR feedback

93aaf6f

PR feedback

92cf18b

Refactored playground state to use a single useState

036251f

Completed refactor

f1c9dbc

Rebase fixes

9a29771

NicholasLYang force-pushed the feature/toggle_tree_view branch from bd71c68 to 9a29771 Compare May 11, 2022 21:41

NicholasLYang temporarily deployed to aws May 11, 2022 21:41 Inactive

fix: Oops formatting

39f04ea

NicholasLYang temporarily deployed to aws May 11, 2022 21:41 Inactive

NicholasLYang merged commit 3d5b0f0 into main May 11, 2022

NicholasLYang deleted the feature/toggle_tree_view branch May 11, 2022 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(website): Toggle Tree Style #2563

feature(website): Toggle Tree Style #2563

NicholasLYang commented May 9, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented May 9, 2022 •

edited

Loading

github-actions bot commented May 9, 2022 •

edited

Loading

IWANABETHATGUY commented May 10, 2022

IWANABETHATGUY commented May 10, 2022

MichaReiser commented May 10, 2022

MichaReiser left a comment

MichaReiser May 10, 2022

NicholasLYang May 10, 2022

MichaReiser May 10, 2022

NicholasLYang May 10, 2022

MichaReiser May 10, 2022

MichaReiser May 10, 2022

MichaReiser May 10, 2022

NicholasLYang May 10, 2022

MichaReiser May 10, 2022

NicholasLYang May 10, 2022

MichaReiser May 11, 2022

NicholasLYang May 11, 2022

MichaReiser May 11, 2022

NicholasLYang May 11, 2022

NicholasLYang May 11, 2022

MichaReiser commented May 11, 2022

feature(website): Toggle Tree Style #2563

feature(website): Toggle Tree Style #2563

Conversation

NicholasLYang commented May 9, 2022 • edited Loading

Summary

Test Plan

cloudflare-workers-and-pages bot commented May 9, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented May 9, 2022 • edited Loading

IWANABETHATGUY commented May 10, 2022

IWANABETHATGUY commented May 10, 2022

MichaReiser commented May 10, 2022

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented May 11, 2022

NicholasLYang commented May 9, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented May 9, 2022 •

edited

Loading

github-actions bot commented May 9, 2022 •

edited

Loading