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

Add command to change fileeditor font size. #4811

Merged
merged 4 commits into from Jul 12, 2018

Conversation

@AlbertHilb
Copy link
Member

@AlbertHilb AlbertHilb commented Jun 29, 2018

This PR is related to #4673.

A command to change font size is added to the fileeditor extension.
That command is then used to add two new items to the Settings menu and to the Text Editor palette section: to increase and decrease, respectively, by one pixel the editor font size.

Cheers

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 29, 2018

Thanks!

CC @tgeorgeux - this changes menu items.

Is it typical to increase/decrease font size by 1 pixel in an editor? I was thinking it may be more typical to change in 2-pixel increments.

@jasongrout jasongrout added this to the Beta 4 milestone Jun 29, 2018
@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Jun 30, 2018

In my experience an increment of two pixels is too great, at least with full HD display.
Atom too uses one pixel increment.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 7, 2018

I agree with the 1px increment. @AlbertHilb, it looks like we picked up a merge conflict, do you mind rebasing?

Use that command to add increase and decrease editor font size items
to the settings menu and to the text editor palette section.
@AlbertHilb AlbertHilb force-pushed the ChangeFontSizeCommand branch from e225f70 to 7959251 Jul 7, 2018
@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Jul 7, 2018

Rebased!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 8, 2018

LGTM, thanks. @jasongrout, I'll let you merge if you agree about the increment.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

I am 👍 on merging this with 1pt increments as well. I had one minor thought which I don't consider blocking.

// Add a command to change font size.
commands.addCommand(CommandIDs.changeFontSize, {
execute: args => {
const delta = args['delta'] as number;
Copy link
Member

@ian-r-rose ian-r-rose Jul 10, 2018

I think this can be a touch more defensive in case the args are not as expected:

const delta = args['delta'] as number || 0;

Copy link
Member Author

@AlbertHilb AlbertHilb Jul 11, 2018

I'm not an expert on TypeScript and therefore I can be wrong (my apologies in that case), but I think the row

const delta = args['delta'] as number || 0;

is transformed to

var delta = args['delta'] || 0;

Provided that args['delta']is evaluated to true, delta value is args['delta'] even if it is not a number.
For example if args['delta'] coincides with the string 'test', then delta value is the string 'test'not the number 0.
So this isn't much more defensive.

Copy link
Member

@ian-r-rose ian-r-rose Jul 11, 2018

True enough, it does not catch delta = 'test', but it does catch the case where delta is null or undefined.

Copy link
Member Author

@AlbertHilb AlbertHilb Jul 11, 2018

In fact it is really useful only in undefined case, nullinside arithmetic operation is already transformed in 0. Maybe something like

const delta = Number(args['delta']);
if (Number.isNaN(delta))
  return;

is more robust. What do you think?

Copy link
Member

@blink1073 blink1073 Jul 11, 2018

Sounds good to me.

Copy link
Member

@ian-r-rose ian-r-rose Jul 11, 2018

Sounds good to me as well. It's not a huge deal, just thinking of the number of times I have been bitten by type mismatches. That's a good point about the coercing (ah, Javascript...).

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented Jul 10, 2018

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jul 11, 2018

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 11, 2018

@ellisonbg, yes, this value is interpreted as a pixel size.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jul 12, 2018

Thanks @AlbertHilb!

@ian-r-rose ian-r-rose merged commit 3ed54cf into jupyterlab:master Jul 12, 2018
2 checks passed
@AlbertHilb AlbertHilb deleted the ChangeFontSizeCommand branch Jul 12, 2018
@jasongrout jasongrout removed this from the 0.34 milestone Jul 19, 2018
@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants