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

Adds & uses hoverintent component #47269

Merged
merged 13 commits into from
Nov 12, 2020
Merged

Adds & uses hoverintent component #47269

merged 13 commits into from
Nov 12, 2020

Conversation

cpapazoglou
Copy link
Contributor

@cpapazoglou cpapazoglou commented Nov 10, 2020

Build on #47238

Changes proposed in this Pull Request

  • Added https://www.npmjs.com/package/react-hoverintent npm package and used it in client/layout/sidebar/expandable.jsx
  • sensitivity, interval={ 90 }, timeout={ 200 } are the same as in wp-admin
  • This change will not affect production unless you use ?flags=nav-unification in URL

Testing instructions

  • add ?flags=nav-unification in URL
  • hover fast the sidebar menus vertically, none should open
  • hover fast the sidebar menus vertically and stay over one randomly, the flyout should open.
  • hover the Setting menu, try moving the cursor to General in a straight line (fast enough). The flyout should not close.
  • hover the Setting menu, try moving the cursor to General in a straight line (slowly). The flyout should close.
  • hover the Posts menu, try moving the cursor to Tags in a straight line(fast enough). The flyout should not close. Links flyout should not open.
Before After
SS 2020-11-10 at 13 48 18 SS 2020-11-10 at 13 49 20

Fixes #46094

@matticbot
Copy link
Contributor

@cpapazoglou cpapazoglou self-assigned this Nov 10, 2020
@cpapazoglou cpapazoglou added [Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 10, 2020
@matticbot
Copy link
Contributor

matticbot commented Nov 10, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~310 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
woocommerce                  +34 B  (+0.0%)      +27 B  (+0.0%)
site-blocks                  +34 B  (+0.0%)      +27 B  (+0.0%)
security                     +34 B  (+0.0%)      +27 B  (+0.0%)
purchases                    +34 B  (+0.0%)      +14 B  (+0.0%)
privacy                      +34 B  (+0.0%)      +27 B  (+0.0%)
notification-settings        +34 B  (+0.0%)      +27 B  (+0.0%)
me                           +34 B  (+0.0%)      +27 B  (+0.0%)
help                         +34 B  (+0.0%)      +27 B  (+0.0%)
happychat                    +34 B  (+0.0%)      +27 B  (+0.0%)
devdocs                      +34 B  (+0.0%)      +26 B  (+0.0%)
account-close                +34 B  (+0.0%)      +27 B  (+0.0%)
account                      +34 B  (+0.0%)      +27 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~1400 bytes added 📈 [gzipped])

name                                           parsed_size           gzip_size
async-load-calypso-reader-sidebar                  +1818 B  (+3.0%)     +469 B  (+3.3%)
async-load-calypso-my-sites-sidebar-unified        +1818 B  (+3.6%)     +448 B  (+3.2%)
async-load-calypso-my-sites-sidebar                +1818 B  (+1.2%)     +457 B  (+1.1%)
async-load-calypso-components-jetpack-sidebar        +34 B  (+0.1%)      +26 B  (+0.4%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@cpapazoglou cpapazoglou force-pushed the fix/flyouts-hoverintent branch 2 times, most recently from 7d25995 to df8fabb Compare November 10, 2020 15:53
@cpapazoglou cpapazoglou force-pushed the fix/flyouts-hoverintent branch from df8fabb to 1ca22ed Compare November 10, 2020 18:00
@cpapazoglou
Copy link
Contributor Author

@scinos are there any hesitations regarding the new package added?

@obenland obenland requested review from a team November 10, 2020 18:32
@mreishus
Copy link
Contributor

mreishus commented Nov 10, 2020

react and react-dom being added to the yarn.lock raise a yellow flag for me - we already have react 16 installed but this is adding react 15 to the lockfile.

2020-11-10_12-43

@cpapazoglou
Copy link
Contributor Author

cpapazoglou commented Nov 10, 2020

react and react-dom being added to the yarn.lock raise a yellow flag for me - we already have react 16 installed but this is adding react 15 to the lockfile.

2020-11-10_12-43

Indeed, would it be better if we just rewrote that file on our own?
https://github.com/nerdchacha/react-hoverintent/blob/master/src/components/index.js

@mreishus
Copy link
Contributor

That sounds like a good idea, since:

  • We only need one file
  • The package is version 0.0.10
  • It hasn't been touched in almost a year

@jsnajdr
Copy link
Member

jsnajdr commented Nov 10, 2020

Another option is to instruct Yarn to force react@15 resolution to the 16.x (and soon 17.x) version that we are using and avoid the duplication. The <HoverIntent> component does only things that are perfectly compatible: componentDidMount, componentWillUnmount, cloneElement.

@scinos is the expert on Yarn-related things. I'm not even sure if this forced resolution is a good idea. Just putting an extra option on the table.

@scinos
Copy link
Contributor

scinos commented Nov 10, 2020

I share the same concerns about bringing in react@15 to the codebase. I think we should consider:

  • Use a different package that depends on react@16 (or even better, declares react as a peer dependency)
  • Raise PRs to react-hoverintent to update react@16 and use it.
  • Fork it.
  • Write our own.

@tyxla
Copy link
Member

tyxla commented Nov 11, 2020

Not much to add to what folks already added here, but I agree we very much should avoid bringing another version of React on the table. Forking it and bumping the React dependency shouldn't be a big deal, because as Jarda pointed out, the React lifecycle methods used in this package should be fully compatible with the ones in a newer React version.

@cpapazoglou
Copy link
Contributor Author

Thanks everybody for chiming in!
I eventually ported the component to calypso/lib, since it seems there is no recent activity (bugs, issues etc) and we need to move fast here.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

I've left some drive-by nits in case you find those helpful 😉

client/layout/sidebar/expandable.jsx Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/README.md Outdated Show resolved Hide resolved
client/lib/hoverIntent/index.js Outdated Show resolved Hide resolved
@mreishus
Copy link
Contributor

I ran prettier on the readme file. [fix/flyouts-hoverintent fb33741] Run prettier on the README file

@frontdevde
Copy link
Contributor

Just noting that I tested this as per testing instructions and from a functional perspective it's working nicely as intended.

@cpapazoglou
Copy link
Contributor Author

I have addressed all feedback, I can't imagine what I would do without you all ❤️, THANKS!

@mreishus
Copy link
Contributor

Below it says " Check code style (Calypso) — TeamCity build failed " failed, but I see no difference in the output of yarn run lint on this branch and master.

Copy link
Contributor

@mreishus mreishus left a comment

Choose a reason for hiding this comment

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

This is working well for me. ✔️

@scinos
Copy link
Contributor

scinos commented Nov 12, 2020

There were a couple of lint errors in the README.md file:
image

I've fixed them, should be green now.

@cpapazoglou cpapazoglou merged commit 2d87a58 into master Nov 12, 2020
@cpapazoglou cpapazoglou deleted the fix/flyouts-hoverintent branch November 12, 2020 07:05
@cpapazoglou cpapazoglou removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nav unification - (Re)position flyout menus based on available screen space
7 participants