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

change font size with command shortcuts #34

Merged
merged 4 commits into from Jul 5, 2016
Merged

change font size with command shortcuts #34

merged 4 commits into from Jul 5, 2016

Conversation

jhaynie
Copy link
Contributor

@jhaynie jhaynie commented Jul 4, 2016

Change the font size for the window with standard command + or -

@montogeek
Copy link
Contributor

command+= is Cmd + +?

@rauchg
Copy link
Member

rauchg commented Jul 4, 2016

This is brilliant. We should probably memorize the option @jhaynie. Perhaps localStorage?

@johanbrook
Copy link
Contributor

👍.

@rauchg Regarding memorizing the option, that should ideally live in some sort of global config somewhere? (out of scope for this PR, but still). Perhaps related to the hyperterm.json mentioned in #14.

@rauchg
Copy link
Member

rauchg commented Jul 4, 2016

On further reflection, it'd be cool to register it as menu accelerators:

image

@rauchg
Copy link
Member

rauchg commented Jul 4, 2016

@johanbrook yes, was thinking exactly that. But in order to make quick progress I'd like to use localStorage for now and merge this sooner rather than later.

alert(e);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This functions are almost identical. I would DRY them up into one that accepted an argument of whether to increment or decrement.

@montogeek
Copy link
Contributor

It should also support switch back to the default size

@jhaynie
Copy link
Contributor Author

jhaynie commented Jul 4, 2016

i'll add the menu accelerators and DRY up the functions. I think we should memoize these in a separate PR to keep this one focused.

@jhaynie
Copy link
Contributor Author

jhaynie commented Jul 4, 2016

OK, i added reset behavior and added window shortcuts and DRY up code.

@rauchg rauchg merged commit 55ac59c into vercel:master Jul 5, 2016
@rauchg
Copy link
Member

rauchg commented Jul 5, 2016

@jhaynie this is beyond beautiful and it works flawlessly. Only nit: we need to let the user know about the font change.

Here's my suggestion (it's shown alongside the columns and rows since font changes trigger a resize)

screen shot 2016-07-04 at 6 46 48 pm

@rauchg
Copy link
Member

rauchg commented Jul 5, 2016

Another minor nit: from the perspective of <HyperTerm> it'd be more elegant if we simply alter <Term fontSize={this.state.fontSize} /> in the render loop.

Then we can capture it in shouldComponentUpdate and manually perform the transformation on the local term reference and return false.

@rauchg
Copy link
Member

rauchg commented Jul 5, 2016

I'm filing issues for these two things.

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.

None yet

5 participants