Skip to content
This repository has been archived by the owner. It is now read-only.

Responsive tab / refactor to Aphrodite #6900

Merged
merged 1 commit into from Feb 7, 2017
Merged

Responsive tab / refactor to Aphrodite #6900

merged 1 commit into from Feb 7, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jan 28, 2017

Test plan

  • Tabs should have a cohesive/pleasant UI with no inconsistent icon alignment/overflow.
  • Tabs should be responsive: resizing them should update each icon to fit its container.
  • Action of mute/unmute audio icon shouldn't change mouse position for each toggle.

Automated tests

Covered by automated tests

npm run test -- --grep="tabContent components"
  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Auditors: @bsclifton, @bbondy

/cc @bradleyrichter

Fix #5431
Fix #6511
Fix #6845
Fix #1776
Fix #100

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Jan 28, 2017

For reference, @bradleyrichter reported some improvements here: #5918 (comment)

@cezaraugusto cezaraugusto force-pushed the tabs-refactor branch 3 times, most recently from c14ad16 to e7fba5b Compare Jan 28, 2017
@luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jan 28, 2017

There is a lot of errors which seems to me to be related with the changes

@bbondy
Copy link
Member

@bbondy bbondy commented Jan 28, 2017

moving to 0.13.2, will consider moving back if tests are fixed up in time. Thanks.

Copy link
Member

@bsclifton bsclifton left a comment

Just now got to reviewing (doh- sorry) ☹️ Needs a rebase, since the siteDetail objects have been reworked.

I did notice this though:
Much of the code is using process.platform for the win32 check. This is not going to work in the renderer, because the renderer process (ex: where the tab runs) does not have access to node.js.

For a safe way to check, take a peek at the platformUtil helper code (specifically the isWindows). This checks the process.platform (which will return undefined) but also checks navigator

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Jan 30, 2017

Once ready, this may also fix #6845

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Jan 30, 2017

@bsclifton thanks added to PR description

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Jan 30, 2017

Updated with test fixes.

The below are intermittent, but passed locally:

npm run test -- --grep="remembers findbar input when switching frames"
npm run test -- --grep="shows no tab pages when you have only 1 page"
npm run test -- --grep="Retains user input on tab switches"

Copy link
Member

@bsclifton bsclifton left a comment

Looking good! I tested on Windows and Mac and found 2 issues (only 1 that I think we need to address)

  1. When you set the max tab count as 20, things can get a little cramped. However, I think this is outside the scope of this change. The behavior with this PR is MUCH better than what we have today. I think this would be a great follow up issue

  2. There was a PR by @gyandeeps which made the tabs stay the same size when closing. This does appear to still work... but the close button is hidden. STR:

  • open up a bunch of tabs
  • close them, starting in the middle
  • notice after closing one that the close icon doesn't automatically show. You have to move mouse off tab and back on. But by doing that, mouse leaves and triggers the resize code.

edit:
here are pics of the cramped area when max tabs is set to 20 and width is smallest; we can use this for creating a follow up issue
screen shot 2017-01-30 at 1 19 21 pm
screen shot 2017-01-30 at 1 34 30 pm

const {StyleSheet} = require('aphrodite')
const globalStyles = require('./global')

const styles = StyleSheet.create({
Copy link
Member

@bsclifton bsclifton Jan 30, 2017

Choose a reason for hiding this comment

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

Really well done here! 😄 This is perfect

tabIcon: '14px',
tabTitle: '12px'
},
appIcons: {
Copy link
Member

@bsclifton bsclifton Jan 30, 2017

Choose a reason for hiding this comment

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

👌

module.exports.isLinux = () => {
return !module.exports.isDarwin() &&
!module.exports.isWindows()
}
Copy link
Member

@bsclifton bsclifton Jan 30, 2017

Choose a reason for hiding this comment

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

this is perfect! For anyone else looking at this... I believe this is the best way because there could be many variations of navigator.platform for Linux. We can't rely on process.platform because this code is often invoked in the renderer (like in this PR)

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Feb 1, 2017

Updated after #6900 (review) regarding sequential tab closing issue.

Copy link
Member

@bsclifton bsclifton left a comment

Other than the feedback I left with small nitpicks, everything else looks great! Awesome job on this one 😄

const locationHasFavicon = this.props.tab.get('location') !== 'about:newtab'
componentDidMount () {
// Execute resize handler at a rate of 15fps
window.addEventListener('resize', throttle(this.onUpdateTabSize, 66))
Copy link
Member

@bsclifton bsclifton Feb 2, 2017

Choose a reason for hiding this comment

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

This 66 magic number may make more sense as a constant in tabUtil.js

// Copy the hover state if tab closed with mouse
// This allow us to have closeTab button visible
// for sequential frames closing, until onMouseLeave event happens.
if (hoverState) {
Copy link
Member

@bsclifton bsclifton Feb 2, 2017

Choose a reason for hiding this comment

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

you may also want to check nextFrame; like if (hoverState && nextFrame) {

z-index: @zindexTabs;
}
}

Copy link
Member

@bsclifton bsclifton Feb 2, 2017

Choose a reason for hiding this comment

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

Seeing all this deleted is beautiful! 😄

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Feb 2, 2017

@bbondy did you want to give this one a go? I think you'll like it 😄

@bbondy
Copy link
Member

@bbondy bbondy commented Feb 4, 2017

wow this is very awesome @cezaraugusto, nice work.

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Feb 6, 2017

@cezaraugusto need your help with 2 things 😄

  • For each of the issues listed, can you please put the test plan at the top of each issue?
  • I noticed two failing tests (broken selectors?). They may not be related, but help looking into them is appreciated 😄
npm run test -- --grep="navigationBar tests submit page that does not load resets URL to previous location if page does not load"

npm run test -- --grep="tab pages basic tab page functionality tabs per page setting takes effect immediately"

@cezaraugusto
Copy link
Contributor Author

@cezaraugusto cezaraugusto commented Feb 7, 2017

@bsclifton failing tests not related to selector. Failing on master as well.

npm run test -- --grep="navigationBar tests submit page that does not load resets URL to previous location if page does not load

Searches for activeWebview, which was not modified.

npm run test -- --grep="tab pages basic tab page functionality tabs per page setting takes effect immediately"

Test is described inside basic tab page functionality, and the only selector which was modified is on before() action, but other tests wrapped there passes as well, so I don't think it's related. It's also failing on master as well.

Also updated target issues with test plan pointing for this PR's top comment.

@bsclifton
Copy link
Member

@bsclifton bsclifton commented Feb 7, 2017

Looks like we have navigationBar failures tracked here #6666

And tab failures tracked here: #6981

Since the rest passed (and these are happening in master), I'll go ahead and merge. Thanks for updating the issues 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants