font size improvements #57
Conversation
- fixed keyboard shortcut registering (listeners were being called twice) - now saving fontSize in the state of Hyperterm - Term now accepts fontSize as a prop and internally sets it - added font size indicator on change (next to rows x cols)
|
nice, great refactor! |
| @@ -89,7 +91,8 @@ export default class HyperTerm extends Component { | |||
| }</div> | |||
| </div> | |||
| <div className={classes('resize-indicator', { showing: this.state.resizeIndicatorShowing })}> | |||
| { this.state.cols }x{ this.state.rows } | |||
| <div>{ this.state.fontSize }px</div> | |||
There was a problem hiding this comment.
does this mean we always show the font size when resizing? perhaps a bit too much info?
There was a problem hiding this comment.
ideally, we only show the font size change when the font size changes, and the cols and rows when the cols and rows change
There was a problem hiding this comment.
yep makes sense, i've made it like you said
| keys.bind('command+=', this.increaseFontSize); | ||
| keys.bind('command+-', this.decreaseFontSize); | ||
| keys.bind('command+plus', this.increaseFontSize); | ||
| keys.bind('command+minus', this.decreaseFontSize); |
There was a problem hiding this comment.
is this still needed since we now use the electron accelerators in index.js ?
There was a problem hiding this comment.
you're quite right (this is probably why the listener was being called twice) fixed
|
Super clean stuff @nfcampos |
| } | ||
|
|
||
| increaseFontSize () { | ||
| this.changeFontSize(1); | ||
| this.changeFontSize(1, true); |
There was a problem hiding this comment.
I'm not the biggest fan of boolean arguments but I didn't want to repeat that setTimeout thing in two functions...
I can change it to this.changeFontSize(1, {increment: true}), should I?
There was a problem hiding this comment.
{ relative: true } ?
Also, are we cleaning up fontSizeIndicatorTimeout on unmount?
There was a problem hiding this comment.
yeah relative is better.
good catch, I'd forgotten about that
I've made both changes
…is now an options object
|
Amazing work @nfcampos |
|
Does this include changing the types of fonts @rauchg? |
|
We're doing that through config, see the other PR. |
|
What PR? |
closes #44 and #45
(bonus improvement: now font size is shared between all tabs)