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

Main Dashboard: improve performance by loading React dash only when connected / in dev mode #13563

Merged
merged 11 commits into from
Oct 15, 2019

Conversation

enejb
Copy link
Member

@enejb enejb commented Sep 27, 2019

Improved the loading performance of the connection page my making sure that we don't load the admin.js file.

This results in a much faster appearance of the connection button the jetpack dashboard page.

Before:
https://cloudup.com/csvrz9kD32W

After:
https://cloudup.com/cvBU_z__CQa

Notes

This makes use of a React portal to hook 2 extra links to the footer, now relying on PHP for all links but those 2.

This introduces 2 issues:

  1. It breaks tracking on the footer links.
  2. It removes the "Reset" and "Dev Tools" buttons when not connected to WordPress.com / not in dev mode.

Testing instructions:

  • Activate the jetpack plugin. Notice that the banner shows up as before.
  • Navigate to the Jetpack Dashboard. Notice that the page loads noticeably faster.
  • Also test when in development mode, and in different connection scenarios.

Proposed changelog entry for your changes:

  • Improves the loading of the dashboard when Jetpack is not connected.

@enejb enejb added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. Connect Flow Connection banners, buttons, ... labels Sep 27, 2019
@enejb enejb added this to the 7.8 milestone Sep 27, 2019
@enejb enejb requested review from zinigor, dereksmart and a team September 27, 2019 13:31
@enejb enejb self-assigned this Sep 27, 2019
@jetpackbot
Copy link

jetpackbot commented Sep 27, 2019

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 7097a29

delawski
delawski previously approved these changes Sep 27, 2019
Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

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

Tested it and works great! The code looks good, too.

tyxla
tyxla previously approved these changes Sep 27, 2019
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.

This did improve the performance in an obvious way. Nice work Enej! 👏

@tyxla tyxla added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 27, 2019
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 30, 2019
Copy link
Member

@jeherve jeherve 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 breaking the dashboard for sites using Jetpack's Development mode.

@enejb
Copy link
Member Author

enejb commented Sep 30, 2019

True the notice is not there any more. Since it is baked into the admin.js bundle which this PR removed on purpose.

To read the notice. I think it would make more sense to rething the notices in the first place.
Right now that notice is not super useful for me.
I think it does help users that run the Jetpack beta version of the plugin but but they already see what version is being run in the admin bar.

cc @keoshi for some ideas here.

@enejb enejb added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 30, 2019
@jeherve
Copy link
Member

jeherve commented Sep 30, 2019

By Development mode, I meant this:
https://jetpack.com/support/development-mode/

The dashboard does not load at all in this mode anymore since we're now only loading the bundle if Jetpack is connected to WordPress.com.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 30, 2019
@keoshi
Copy link
Contributor

keoshi commented Sep 30, 2019

@enejb and I just chatted a bit about this and I agree that we can do without the notice, especially if Jetpack is much more performant without loading these assets.

A proposed solution would be to add a little flag at the top of the page, as well as note if the user is using Beta/staging environment/etc. in the footer.

Quick and dirty mock:

image

One thing left to consider is that we'd be removing the Submit Beta feedback link. In p1569845703354400-slack-jpop-support I've asked if we have any data on the amount of feedback we receive. Depending on that we might add the link to the top or bottom.

@jeherve
Copy link
Member

jeherve commented Sep 30, 2019

Quick and dirty mock:

We can talk about this a bit more in a different issue, but it's worth noting that we already have such badges when the site is connected to a WordPress.com sandbox:

image

--

sandboxedBadge = this.props.sandboxDomain ? (

We also have this notice when using Jetpack Dev mode:

image

--

const devNotice = this.props.siteConnectionStatus === 'dev' ? <code>Dev Mode</code> : '',

@enejb
Copy link
Member Author

enejb commented Sep 30, 2019

Thanks for the clarification and great work catching that! I didn't test it in that mode at all.

@enejb enejb dismissed stale reviews from tyxla and delawski via cbee2d0 September 30, 2019 12:40
@enejb
Copy link
Member Author

enejb commented Sep 30, 2019

Thanks the fix you suggested works as tested.
However now when I am not connected but a user that can't connect I end up stairing at the blank screen.
I wonder if we should show a nicer notice to the user in that case.
Or not even allow the user to end up there.

@jeherve
Copy link
Member

jeherve commented Sep 30, 2019

This change also causes some footer links (including the Debug link that can be used to diagnose connection issues) to disappear:

Before

image

After

image

We may want to keep those, even for unconnected users.

@enejb enejb added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Oct 9, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Would it be an option to drop the whole footer implementation from the React dashboard, and only use PHP to display that footer?

We could then do the tracking from the PHP / JS side as well, since we load the tracks js client on all pages already.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 9, 2019
@enejb enejb added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 9, 2019
@jeherve
Copy link
Member

jeherve commented Oct 14, 2019

I pushed a change to remove a duplicate, and extract the link building process. I was hoping that this would help me bring Tracks back on those links. Unfortunately it is not that easy. While I can add some tracking with the data-jptracks-name and data-jptracks-prop attributes on links, those props won't exactly match the ones that were defined when the links were built within the React interface.

Another problem this PR introduces is the lack of "Reset" and "Dev Tools" options when the site is not connected to WordPress.com. Those buttons are useful when developing Jetpack.

None of this may be blocking, especially since this PR will introduce significant performance improvements for folks connecting Jetpack to WordPress.com for the first time. That said, I'd love to have another Crew member's final opinion on this.

@jeherve jeherve changed the title Update/connect in place loading perf Main Dashboard Page: improve performance by loading React dash only when connected / in dev mode Oct 14, 2019
@jeherve jeherve changed the title Main Dashboard Page: improve performance by loading React dash only when connected / in dev mode Main Dashboard: improve performance by loading React dash only when connected / in dev mode Oct 14, 2019
@kraftbj
Copy link
Contributor

kraftbj commented Oct 14, 2019

Another problem this PR introduces is the lack of "Reset" and "Dev Tools" options when the site is not connected to WordPress.com. Those buttons are useful when developing Jetpack.

I would consider that non-blocking for this PR (the performance enhancement for regular users is worth it) but we can/should re-add them in another PR. Either in the footer there or the Jetpack Debug page (which I should take a pass at now that most interesting bits of that page are in the Site Health now).

@enejb enejb requested a review from tyxla October 15, 2019 08:36
@enejb enejb merged commit 71d0a1f into master Oct 15, 2019
@enejb enejb deleted the update/connect-in-place-loading-perf branch October 15, 2019 08:47
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 15, 2019
@tyxla
Copy link
Member

tyxla commented Oct 15, 2019

Nice work folks 👏

jeherve added a commit that referenced this pull request Oct 23, 2019
jeherve added a commit that referenced this pull request Oct 23, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Connect Flow Connection banners, buttons, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants