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

allow dynamic changing the font-size of the notes #428

Closed
wants to merge 3 commits into from
Closed

allow dynamic changing the font-size of the notes #428

wants to merge 3 commits into from

Conversation

chrisns
Copy link
Contributor

@chrisns chrisns commented Feb 18, 2022

Feb-18-2022 18-25-41

buttons that appear when hovering over the notes in presenter view, change the font-size by +/- 0.1em

@yhatt yhatt self-requested a review February 19, 2022 05:16
Copy link
Member

@yhatt yhatt left a comment

Choose a reason for hiding this comment

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

Probably I'll retry to make UI in another pull request based on this review for better user experience.

Comment on lines +113 to +114
if (e.key === '+' || e.key === '=') resizeNotes('bigger')
if (e.key === '-' || e.key === '_') resizeNotes('smaller')
Copy link
Member

Choose a reason for hiding this comment

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

= and _ are not required.

In my keyboard layout, there is no _ key assign in - keycap. Instead, - key has = key assign.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On my uk keyboard it looks like this. The plus requires holding shift.
The extra key binding while not correct felt more intuitive
image

Copy link
Member

Choose a reason for hiding this comment

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

I think it is not intuitive for me because my key layout will trigger unexpected scaling by not related key.

IMG_2531

resizeNotes('smaller')
)

document.addEventListener('keyup', (e: KeyboardEvent) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should use keydown event instead of keyup.

Check e.repeat within event callback to ignore key repeat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something else is binding to keydown and stopping propagation.
What do you advise?

Copy link
Member

@yhatt yhatt Feb 19, 2022

Choose a reason for hiding this comment

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

The element for notes is stopping event propagation because global slide page navigation will conflict with arrow keys for scrolling notes. #431 has enabled useCapture flag in third argument:

true // Ignore stopped propagation in notes

Comment on lines +96 to +100
const resizeNotes = (direction: string) => {
const current = parseFloat($(classes.noteContainer).style.fontSize) || 1
const intended = current + (direction === 'bigger' ? 0.1 : -0.1)
$(classes.noteContainer).style.fontSize = `${intended.toFixed(1)}em`
}
Copy link
Member

Choose a reason for hiding this comment

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

Using CSS custom property would be smart as like as #427 (comment).

const notesFontSizeBase = 0
const resizeNotesFont = (increment: number) => {
  notesFontSizeBase = Math.max(-0.5, increment + notesFontSizeBase) // Prevent making too smaller
  $(classes.noteContainer).style.setProperty('--bespoke-marp-note-font-base', notesFontSizeBase.toFixed(1))
}

$(classes.noteButtonsBigger).addEventListener('click', () => resizeNotesFont(0.1))
$(classes.noteButtonsSmaller).addEventListener('click', () => resizeNotesFont(-0.1))
.bespoke-marp-note {
  /* Base font size is 1.1em, and add modified difference of font size by user */
  font-size: calc((1.1 + var(--bespoke-marp-note-font-base, 0)) * 1em);
}

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><path fill="none" stroke="#fff" stroke-linecap="round" stroke-linejoin="round" stroke-width="5" d="M92,70L52,30L12,70"/></svg>
Copy link
Member

Choose a reason for hiding this comment

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

Chevron marks may confuse users who were thinking them as to scroll notes. Modern browsers are using - and + icons as to scale so we should follow common style by creating new icons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll admit I was being lazy and just rotating the ones you already had😂, do you have an easy way to make +&- that match?

Copy link
Member

Choose a reason for hiding this comment

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

I made new designs for plus and minus icon at #431.

yhatt added a commit that referenced this pull request Feb 19, 2022
@yhatt
Copy link
Member

yhatt commented Feb 19, 2022

-> #431

@yhatt yhatt closed this Feb 19, 2022
yhatt added a commit that referenced this pull request Feb 19, 2022
…otes

Polished #428: Make notes font size changeable in bespoke template
@yhatt
Copy link
Member

yhatt commented Feb 19, 2022

A improved version #431 has been merged. Thank you for your nice suggestion and contribution 😄

@chrisns chrisns deleted the change_fontsize_of_notes branch April 8, 2022 11:08
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.

2 participants