Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Add an option for "no delay" in tab preview timing #12832

Merged
merged 2 commits into from
Jan 24, 2018
Merged

Add an option for "no delay" in tab preview timing #12832

merged 2 commits into from
Jan 24, 2018

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jan 24, 2018 •
edited
Loading

closes #8860

this is a follow-up of #9887 and adds a none option for tab preview delay.

Test Plan:

  1. Go to pref->tabs
  2. Confirm tab preview defaults to "short"
  3. Change tab preview timing to "none"
  4. tab previews should be instant

@cezaraugusto cezaraugusto self-assigned this Jan 24, 2018
@cezaraugusto cezaraugusto added this to the 0.20.x (Beta Channel) milestone Jan 24, 2018
@codecov-io
Copy link

codecov-io commented Jan 24, 2018 •
edited
Loading

Codecov Report

Merging #12832 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #12832   +/-   ##
=======================================
  Coverage   56.13%   56.13%           
=======================================
  Files         279      279           
  Lines       27309    27309           
  Branches     4442     4442           
=======================================
  Hits        15329    15329           
  Misses      11980    11980
Flag Coverage Δ
#unittest 56.13% <100%> (ø) ⬆️
Impacted Files Coverage Δ
js/constants/appConfig.js 100% <100%> (ø) ⬆️
app/renderer/components/preferences/tabsTab.js 100% <100%> (ø) ⬆️
app/common/constants/settingsEnums.js 100% <100%> (ø) ⬆️

petemill
petemill reviewed Jan 24, 2018
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

Works great, left 1 question. I think it's 'instant' enough to consider it remaining being described as 'no delay' although perhaps could try reducing pause to somewhere between 25ms and 40ms?

app/renderer/components/tabs/tabs.js Outdated
@@ -195,6 +196,7 @@ class Tabs extends React.Component {
key={'tab-' + frameKey}
ref={(node) => this.tabRefs.push(node)}
frameKey={frameKey}
tabsPerTabPage={this.props.tabsPerTabPage}
Copy link
Member

Choose a reason for hiding this comment

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

Wondering why a new prop has been added here, but no change to the Tab component to utilize it. Is this fixing a different bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh my bad this was for another pr thanks for spotting

@cezaraugusto
Copy link
Contributor Author

@petemill ya I like instant and also suggested immediate but I guess @bradleyrichter is searching for a better phrase for the select title.

re timing I'm ok changing to 40 btw but can't really spot the difference. Below 30ms I can repro the unwanted flash so I'd go 40-50ms

@Jacalz
Copy link
Contributor

Jacalz commented Jan 24, 2018 •
edited
Loading

It is getting late in Sweden so I will veck this one out early tomorrow 🙂

I was thinking of names and maby it could be called “Classic” or something because it is the old style 😅

bsclifton
bsclifton reviewed Jan 24, 2018
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Timing looks great to me 👍 Definitely like the naming too (No delay). Nice fix! 😄

bsclifton
bsclifton previously approved these changes Jan 24, 2018
View reviewed changes
@cezaraugusto cezaraugusto changed the title Add an option for NO_DELAY in tab preview timing Add an option for "no delay" in tab preview timing Jan 24, 2018
bsclifton
bsclifton reviewed Jan 24, 2018
View reviewed changes
js/constants/appConfig.js
@@ -153,7 +153,7 @@ module.exports = {
'tabs.tabs-per-page': 20,
'tabs.close-action': 'parent',
'tabs.show-tab-previews': true,
'tabs.preview-timing': 1000,
'tabs.preview-timing': tabPreviewTiming.SHORT,
Copy link
Member

Choose a reason for hiding this comment

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

nice!! Love this 😄

bsclifton
bsclifton approved these changes Jan 24, 2018
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

I tried all 3 options- each look and feel great 👍 Thanks for tweaking this (and thanks for bringing this up, @Jacalz !! )

petemill
petemill approved these changes Jan 24, 2018
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

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

nice and clean! 👏

@petemill petemill merged commit 16740dc into master Jan 24, 2018
@bsclifton bsclifton deleted the tabs/preview/8860-2 branch January 24, 2018 23:44
@petemill
Copy link
Member

master 16740dc
0.21.x 9ef9571 4b91dfe
0.20.x 9ef9571 4b91dfe

@Jacalz
Copy link
Contributor

Jacalz commented Jan 25, 2018 •
edited
Loading

Can’t get the browser to build because I reinstalled my computer 🖥Going to need to wait for a new beta build from @bsclifton 🙂

Jacalz
Jacalz reviewed Jan 25, 2018
View reviewed changes
Copy link
Contributor

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

This feels and works almost exactly like the old system and it even feels slightly better, great job 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Jacalz Jacalz Jacalz left review comments

@petemill petemill petemill approved these changes

@bsclifton bsclifton bsclifton approved these changes

Assignees

@cezaraugusto cezaraugusto

Labels
feature/preferences feature/tab-preview
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

Successfully merging this pull request may close these issues.

Make tab preview based on mouse idle time
5 participants
@cezaraugusto @codecov-io @Jacalz @petemill @bsclifton