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

contextual zoom font size adjustment #376

Merged
merged 7 commits into from
Jul 22, 2023
Merged

Conversation

chimmyten
Copy link
Contributor

When contextual zoom is active, users can adjust the font size using a slider/input box in the sidebar. Fixes #345. Please let me know any feedback.

Zoomed.Font.Size.Adjustment.Small.mov

@lihebi
Copy link
Collaborator

lihebi commented Jul 17, 2023

Very cool!

You used one slider to control the font-size of all scope levels. We actually want to have different fontSize for different levels. The fontSize variable computes the different font sizes:

const fontSize = level2fontsize(node?.data.level);

So, it's better to use separate sliders to control each scope level individually. I.e., add six sliders to control and set the six font sizes 56, 48, 32, 24, 16, 8 here.

contextualZoomParams: {
prev: 56,
0: 48,
1: 32,
2: 24,
3: 16,
next: 8,
threshold: 16,
},
level2fontsize: (level: number) => {
// default font size
if (!get().contextualZoom) return 16;
// when contextual zoom is on
switch (level) {
case -1:
return get().contextualZoomParams.prev;
case 0:
return get().contextualZoomParams[0];
case 1:
return get().contextualZoomParams[1];
case 2:
return get().contextualZoomParams[2];
case 3:
return get().contextualZoomParams[3];
default:
return get().contextualZoomParams.next;
}
},

@forrestbao
Copy link
Collaborator

forrestbao commented Jul 17, 2023

But are separate sliders for different levels too much? Would letting users set the minimal font be more manageable?

@lihebi
Copy link
Collaborator

lihebi commented Jul 17, 2023

At this stage, we want to tune the parameters (ourselves). Making all font sizes configurable can better suit this need. The sliders won't show when "contextual zoom" is not enabled, so it won't be that visible.

@forrestbao
Copy link
Collaborator

Good point. @lihebi

@chimmyten
Copy link
Contributor Author

I could also make all of them only input boxes if there are concerns with sliders taking up too much space.

@forrestbao
Copy link
Collaborator

I like input boxes, as long as I can quickly use arrow keys on my keyboard to increase or decrease the value. keystrokes are more effective than mouse drags.

@lihebi
Copy link
Collaborator

lihebi commented Jul 17, 2023

I also thought about using only input boxes to save space last night. But i also liked the sliders for quick operation and more explicit visual appearance.

Let’s have both the sliders and input boxes in this PR? This setting is at an alpha stage, so we can have some redundant UI here. We can refine the UI once it is ready to release.

@chimmyten
Copy link
Contributor Author

Sounds good 👍

@li-xin-yi
Copy link
Collaborator

Maybe we can consider hotkeys for those operations at the same time. For example, ctrl/command + shift + +/-.

@lihebi
Copy link
Collaborator

lihebi commented Jul 17, 2023

Yeah, let's add some shortcuts when we have fewer sliders in the future.

@chimmyten
Copy link
Contributor Author

Hello, I'm a bit confused about the first property of the contextualZoomParams. https://github.com/chimmyten/codepod/blob/main/ui/src/lib/store/settingSlice.tsx#L85C3-L93C5
On which level would a pod have the "prev" font size? Base-level pods, i.e. pods not placed in any scopes seem to adhere to the "0" font size.

@lihebi
Copy link
Collaborator

lihebi commented Jul 19, 2023

You are right; the .prev seems not used/needed. You can ignore it or remove it entirely.

Essentially we just need levels 0,1,2,3 and a default value for all levels beyond 3.

@chimmyten
Copy link
Contributor Author

Got it, thank you.

@chimmyten
Copy link
Contributor Author

I added different sliders to control font sizes for different levels.

AdjustableFontSizeSliders.Small.mov

A couple of notes/questions:
I ran into this strange behavior where the font sizes of the pods wouldn't actually update with the sliders unless I declared this line of code: https://github.com/chimmyten/codepod/blob/817eb1ff0841b8a14b053997c9ba8618768fb5d2/ui/src/components/nodes/Code.tsx#L565-L566. I couldn't figure out why it was affecting the behavior because I don't actually use this line of code anywhere else. If anyone can explain why that would be great.

I commented out a couple lines of code to prevent the pods from disappearing when the font sizes got too small. https://github.com/chimmyten/codepod/blob/817eb1ff0841b8a14b053997c9ba8618768fb5d2/ui/src/components/nodes/Code.tsx#L587-L594.

Please let me know if there are any issues.

@lihebi
Copy link
Collaborator

lihebi commented Jul 21, 2023

I ran into this strange behavior where the font sizes of the pods wouldn't actually update with the sliders unless I declared this line of code: https://github.com/chimmyten/codepod/blob/817eb1ff0841b8a14b053997c9ba8618768fb5d2/ui/src/components/nodes/Code.tsx#L565-L566. I couldn't figure out why it was affecting the behavior because I don't actually use this line of code anywhere else. If anyone can explain why that would be great.

The reason is that when your slider changes store.contextualZoomParams, the Code/Rich/Scope component won't re-render.

Your contextualZoomParams = useStore(...) line did the trick because it essentially tells React to re-render the component when contextualZoomParams changes. But this is error-prone because people won't easily see why this line is needed and thus might remove it accidentally.

The preferred way is to remove store.level2fontsize and make the usage of contextualZoomParams explicit:

// utils.ts:
export function level2fontsize(level, contextualZoomParams) {...} // move level2fontsize out of Zustand store

// Code.tsx/Rich.tsx/Scope.tsx:
import {level2fontsize} from "./utils"
function Code() {
  ...
  contextualZoomParams = useStore(...)
  ...
  const fontSize = level2fontsize(node?.data.level, contextualZoomParams);
}

@chimmyten
Copy link
Contributor Author

Thank you, that makes sense. I'll make the changes.

@chimmyten chimmyten marked this pull request as draft July 22, 2023 11:15
@chimmyten chimmyten marked this pull request as ready for review July 22, 2023 12:42
@lihebi
Copy link
Collaborator

lihebi commented Jul 22, 2023

I commented out a couple lines of code to prevent the pods from disappearing when the font sizes got too small. https://github.com/chimmyten/codepod/blob/817eb1ff0841b8a14b053997c9ba8618768fb5d2/ui/src/components/nodes/Code.tsx#L587-L594.

Good catch. These are indeed not needed, because a collapsed scope won't have their children rendered at all.

@lihebi
Copy link
Collaborator

lihebi commented Jul 22, 2023

I applied a little clean up and code formatting. Thanks, Timmy!

@lihebi lihebi merged commit 4e9394c into codepod-io:main Jul 22, 2023
@chimmyten
Copy link
Contributor Author

I forgot to link the issue, but this PR should fix #316.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants