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

Fix pane zoom #2532

Open
wants to merge 9 commits into
base: canary
Choose a base branch
from
Open

Conversation

EdgarACarneiro
Copy link
Contributor

@EdgarACarneiro EdgarACarneiro commented Dec 8, 2017

This PR fixes #1496 and is related to #2447 and #2448.
It enables the user to zoom in/out on a single pane, when on split pane mode.

The PR was tested in both Linux and Windows, and works as expected on both.

It relies (branches from) PR #2448, where the global zoom in/out was fixed.

This PR is ready to be merged.

GIF of the feature implemented in Windows:

Imgur

@AndreFCruz
Copy link
Contributor

AndreFCruz commented Dec 9, 2017

I just tested it on macOS and the keybindings don't seem to trigger for me (either this or the cmd+shift+plus also triggers the cmd+plus).
This is due to an unrelated moustrap issue (mousetrap#300), so we decided to change the macOS keybindings until the moustrap issue is fixed (I realise this is not ideal but another option would be to only have this feature for windows and linux).

So, for now, the keybindings are:
Windows and Linux:
Ctrl, + / - for global zoom in/out.
Ctrl, 0 for global zoom reset.
Ctrl, Shift, + / - for zooming in/out on the current pane.
Alt, 0 for resetting the active pane's zoom.

MacOS:
Cmd, + / - for global zoom in/out.
Cmd, 0 for global zoom reset.
Cmd, P for zooming in on the current pane.
Cmd, L for zooming out on the current pane.
Alt, 0 for resetting the active pane's zoom.

Any feedback is appreciated.

@ghost
Copy link

ghost commented Jan 9, 2018

Consider this jargon:

from: https://en.wikipedia.org/wiki/Numeric_keypad#/media/File:Qwerty.svg

In Firefox and Chrome running on Windows,

  1. Ctrl, + zooms in when pressed on:
    a. numeric keypad
    b. typewriter keys
  2. Ctrl, - zooms out when pressed on:
    a. numeric keypad
    b. typewriter keys
  3. 0 zooms to normal when pressed on:
    a. numeric keypad
    b. typewriter keys

In IE and Edge, except for 3a., the rest work.

In Hyper (default installation), 1b, 2a and 3a don't seem to work, the rest do.

@Stanzilla
Copy link
Collaborator

@EdgarACarneiro Hey there, care to resolve the conflicts?

@Stanzilla
Copy link
Collaborator

@chabou

Copy link
Collaborator

@chabou chabou left a comment

Choose a reason for hiding this comment

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

Very nice work!
But some details have to be changed

}
});
};
}
Copy link
Collaborator

@chabou chabou Jan 13, 2018

Choose a reason for hiding this comment

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

I think you don't need using effect mechanism and to add SESSION_FONT_SIZE_INCR and SESSION_FONT_SIZE_DECR action types.

Something like this should work:

export function increaseSessionFontSize() {
  return (dispatch, getState) => {
	const {sessions} = getState();
    const uid = sessions.activeUid;
    const old = sessions.sessions[uid].fontSizeOverride;
    const value = old + 1;
    dispatch({
      type: SESSION_FONT_SIZE_SET,
      uid,
      value
    });
  };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chabou I already removed the effect mechanism of the session font size increase and decrease. Do you want me to do it for the session font size reset too?

style={{
padding: this.props.padding,
fontSize: this.props.fontSize,
WebkitFontSmoothing: this.props.fontSmoothing ? this.props.fontSmoothing : ''
Copy link
Collaborator

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 needed anymore (because of xterm3 integration since 2.0.0-canary.9)
Can you try without theses lines?

@chabou
Copy link
Collaborator

chabou commented Jan 13, 2018

Can you add some menu entries to help users to discover your feature and keymaps?

@EdgarACarneiro
Copy link
Contributor Author

EdgarACarneiro commented Jan 27, 2018

About the menu entries, where are those supposed to go?
@Stanzilla or @chabou

Btw, sorry it took so long to make the requested changes, exams and stuff 😞

@Stanzilla
Copy link
Collaborator

Urm not sure what @chabou meant since there are already menu items for zoom and this PR just fixes it to work correctly in a multi pane scenario, no?

@EdgarACarneiro
Copy link
Contributor Author

@Stanzilla I just wanted to know where the menu entries were to complete them... maybe with the keymaps or some stuff, or even check if the entries are complete! Could you help me here?

@Stanzilla
Copy link
Collaborator

Sure, keymaps are here https://github.com/zeit/hyper/tree/canary/app/keymaps

@EdgarACarneiro
Copy link
Contributor Author

@chabou What menu entries are we talking about? In the hyper.js config file? In the keymaps? I'm not sure I follow

@Stanzilla Stanzilla added 👀 Awaiting Response Issue or PR is awaiting a response from the author 🐧 Platform: Linux Issue pertains to Linux 🖼 Platform: Windows Issue pertains to Windows labels Apr 24, 2018
@Stanzilla
Copy link
Collaborator

Stanzilla commented Apr 24, 2018

Hey there, @EdgarACarneiro!

This needs a rebase, are you willing to pick this up again? I'm sure @chabou will assist you with any questions that you might have :)

@Stanzilla
Copy link
Collaborator

@EdgarACarneiro willing to pick this up again?

@EdgarACarneiro
Copy link
Contributor Author

Hey @Stanzilla , I'm currently with a very tight schedule 😞

@AndreFCruz , care to give it a try?

@AndreFCruz
Copy link
Contributor

AndreFCruz commented Oct 1, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 Awaiting Response Issue or PR is awaiting a response from the author 🐧 Platform: Linux Issue pertains to Linux 🖼 Platform: Windows Issue pertains to Windows WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zoom problem
5 participants