-
Notifications
You must be signed in to change notification settings - Fork 585
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
Scale Tutorial Text Alongside Workspace #9729
base: master
Are you sure you want to change the base?
Changes from all commits
462f987
1f56f2f
8da311f
c7f0bdc
30b0a2c
1811886
3e60074
fff1d72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -152,6 +152,10 @@ export class ProjectView | |
private rootClasses: string[]; | ||
private pendingImport: pxt.Util.DeferredPromise<void>; | ||
|
||
private initialEditorScale: number; | ||
private tutorialInitialFontSize = 1.125; // rem | ||
private tutorialMaxFontSize = 2; // rem | ||
|
||
private highContrastSubscriber: data.DataSubscriber = { | ||
subscriptions: [], | ||
onDataChanged: () => { | ||
|
@@ -211,6 +215,7 @@ export class ProjectView | |
this.exitTutorial = this.exitTutorial.bind(this); | ||
this.setEditorOffset = this.setEditorOffset.bind(this); | ||
this.resetTutorialTemplateCode = this.resetTutorialTemplateCode.bind(this); | ||
this.onScaleChanged = this.onScaleChanged.bind(this); | ||
this.initSimulatorMessageHandlers(); | ||
|
||
// add user hint IDs and callback to hint manager | ||
|
@@ -1029,7 +1034,10 @@ export class ProjectView | |
} | ||
this.allEditors = [this.pxtJsonEditor, this.gitjsonEditor, this.blocksEditor, this.serialEditor, this.assetEditor, this.textEditor] | ||
this.allEditors.forEach(e => e.changeCallback = changeHandler) | ||
this.allEditors.forEach(e => e.onScaleChanged = this.onScaleChanged) | ||
|
||
this.editor = this.allEditors[this.allEditors.length - 1] | ||
this.initialEditorScale = undefined; | ||
} | ||
|
||
public UNSAFE_componentWillMount() { | ||
|
@@ -1108,6 +1116,8 @@ export class ProjectView | |
// subscribe to user preference changes (for simulator or non-render subscriptions) | ||
data.subscribe(this.highContrastSubscriber, auth.HIGHCONTRAST); | ||
data.subscribe(this.cloudStatusSubscriber, `${cloud.HEADER_CLOUDSTATE}:*`); | ||
|
||
document.getElementById("root").style.setProperty("--tutorialFontSize", `${this.tutorialInitialFontSize}rem`); | ||
} | ||
|
||
public componentWillUnmount() { | ||
|
@@ -1532,6 +1542,26 @@ export class ProjectView | |
} | ||
} | ||
|
||
onScaleChanged(oldScale: number, newScale: number) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. passing in the oldScale here to set the initial scale seems kinda fragile. is the initial scale not always the same value? and failing that, can't we just read it from blockly when we load the workspace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's different between Monaco and Blockly. I'll see about reading it when we load the workspace. Should be do-able. |
||
if (this.isTutorial && oldScale !== newScale) { | ||
if (this.initialEditorScale === undefined) { | ||
this.initialEditorScale = oldScale; | ||
} | ||
|
||
const root = document.getElementById("root"); | ||
if (root && newScale <= this.initialEditorScale) { | ||
// Do not shrink the text beyond its initial size. | ||
root.style.setProperty("--tutorialFontSize", `${this.tutorialInitialFontSize}rem`); | ||
} else if (root) { | ||
// Increase font size to match the editor's % zoom. | ||
const maxEditorScale = this.editor.getMaxScale(); | ||
const zoomAmt = (newScale - this.initialEditorScale) / (maxEditorScale - this.initialEditorScale); | ||
const newTutorialFontSize = this.tutorialInitialFontSize + (this.tutorialMaxFontSize - this.tutorialInitialFontSize) * zoomAmt; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain this math a bit? I'm having a hard time understanding why we want to multiply zoom amount with the difference of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be tricky without a whiteboard, but I'll give it a try :) So the question we're trying to answer is how far between the minimum (initial) font and the maximum font we want the new font size to be. To do this, we're basically mapping how far between the editor's min and max scale we are (as a fraction between 0 and 1) and we want to be that same fraction between the font's min and max. We start with the editor's Then to get the font size from that, we kind of do the opposite. We need to figure out where, between the Here's a diagram of questionable value: Also happy to hop on a call and talk through it, if that'd be helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this explanation was extremely helpful! Thank you for that clarification!! |
||
root.style.setProperty("--tutorialFontSize", `${newTutorialFontSize}rem`); | ||
} | ||
} | ||
} | ||
|
||
/////////////////////////////////////////////////////////// | ||
//////////// Load header ///////////// | ||
/////////////////////////////////////////////////////////// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would define this in css instead of adding it here and just override it when the scale changes. that way it'll always have at least some value... anything set on the root directly will override whatever is in the css styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I went back and forth on this. Decided to put it here to keep it all in once place, but happy to move it back to css.