Add an active class on current session panel #905

Merged
merged 3 commits into from Nov 19, 2016

Projects

None yet

5 participants

@weslleyaraujo
Contributor
weslleyaraujo commented Oct 19, 2016 edited

With an active class, users can easily add styles to panels in order to highlight the active session

e.g:

highlight

@chabou
Contributor
chabou commented Oct 19, 2016

I may be wrong but Session seems to be too abstract to put it as a css class or layout subdivision.
term_active seems to be more suitable imo

@rauchg
Contributor
rauchg commented Oct 19, 2016

I wonder if it'd be better to have such classnames in the wrapping component? ie: the component where the <Term> is included

@weslleyaraujo
Contributor

term_active seems to be more suitable imo

@chabou, totally. it does make more sense, I am up for using term_active instead

I wonder if it'd be better to have such classnames in the wrapping component?

@rauchg as far as I understood, Term is where the terminal session itself is being rendered, that's why I used that component. I think it may also live in TermGroup right?

I am quite new to the codebase, if you guys point me in the right direction I would be happy to apply the changes

@chabou
Contributor
chabou commented Oct 20, 2016

@rauchg I think it is better to limit layers that can be tweaked. A lot of style config are for tuning Hterm. And it could be better to stick to this philosophy. A maximum of styling properties (cursor type, cursor color, background, fontsize etc...) should be own by hterm itself (and its iframe).
The less css will be used by users, the more plugins will be compatible with userconfig.

If easily configuring active term is a thing, we could think to have a config section active_term which automatically override props. If it is not, we could make a plugin to achieve this. But expose a css class seems to be weird. What about only use a css file and remove config file ? 😆

I'm currently working on a plugin to autoprofile a Term to distinguish my terms. Style is applied with shell criteria : user, host, path. Exactly like iTerm2 : https://www.iterm2.com/img/screenshots/v3-screen-shots/iterm2-automatic-profile-switching.gif Asking to have a css class with these criteria is not an option. I think that active term criteria should be treated in the same way : core functionality + config file or plugin.

My 2 cents.

@weslleyaraujo
Contributor

@chabou well the active_term section makes sense, I wonder if this is the way to go.. if so I can close this pr and start putting some effort on it 👍

@chabou
Contributor
chabou commented Oct 21, 2016

To be clear I don't want to use a peremptory tone (if it's look like).
I'm french and it's difficult to me to express myself in a balanced manner.

this is just my very very very humble point of view.

@rauchg
Contributor
rauchg commented Oct 22, 2016

I do think having the term_active class is very useful for arbitrary CSS extension. In general, I favor having properties for the things that are likely to be changed. But the only way we can learn about what customizations users want is by exposing class names and allowing arbitrary style mutations.

@rauchg
Contributor
rauchg commented Oct 22, 2016

In short, if you set term_active in the wrapping component that'd be great!

@weslleyaraujo
Contributor

Nice insights guys :)

Well, I will move this class to the wrapper component then, and let's see if it will be useful for someone else 👍

@weslleyaraujo
Contributor

So I am not 100% sure on where exactly this class should live, my understood of the components structure is the following:

<Term />
  • the terminal component itself
<TermGroups />
  • a group of terminals aka a tab
  • has a terms_termGroupActive class when active
  • will render <Term /> as children when only one session is available
  • will render a <SplitPane /> as children when multiple sessions are available
<SplitPane />
  • a wrapper that will render a <Term /> component as children

So if this is correct the options that come to my mind is:

1 . term_active can live in the <Term /> component, which is the deepest level and always will exist no matter users has or not split panels. (that is currently implemented)
2 . term_active can live in the <SplitPane /> component, which I would rather name it differently such as split_active or something similar. In this case, this class would only exist if users have at least one split panel.

So I renamed the class from active_session to term_active, in case we go for option 1. If there is a different way to do it/approach it I would be happy to help 😊

lib/components/term-group.js
@@ -48,6 +48,7 @@ class TermGroup_ extends Component {
const session = this.props.sessions[uid];
const termRef = this.props.terms[uid];
const props = getTermProps(uid, this.props, {
+ isSessionActive: uid === this.props.activeSession,
@chabou
chabou Oct 23, 2016 Contributor

Maybe it is better to have a isActive or isTermActive prop. A Term has no knowledge about what a Session is.

@weslleyaraujo
weslleyaraujo Oct 23, 2016 Contributor

It makes sense.. isTermActive sounds like a good and explicit name for me, will rename it to that :)

@weslleyaraujo
Contributor

term_active

apparently working just fine 👍

@weslleyaraujo
Contributor

friendly ping: any plans on merging this or something else to be discussed over?

@matheuss

Works perfectly 👌

@matheuss
Collaborator

Thank you @weslleyaraujo 🙌

@matheuss matheuss merged commit 47f01f9 into zeit:master Nov 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@weslleyaraujo
Contributor

💪

@weslleyaraujo weslleyaraujo added a commit to weslleyaraujo/hyper that referenced this pull request Nov 20, 2016
@weslleyaraujo weslleyaraujo fix(active-session): rename active session class
see #905
6c88d2b
@paulmolluzzo

Another small ping to ask if a new release is coming soon. 🙏 Looking forward to this PR specifically.

@rauchg
Contributor
rauchg commented Dec 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment