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

Enable preboot event replay for shorter apparent TTI #1761

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ybnd
Copy link
Member

@ybnd ybnd commented Aug 11, 2022

References

Description

This (work-very-much-in-progress) PR continues the integration of Angular preboot into DSpace by enabling event replay, which further improves perceived loading performance by allowing (limited) interaction with the page during the SSR→CSR transition, e.g.:

  • Text typed into form inputs can be preserved
  • Clicks on internal links can be buffered until the app is loaded
    • This results in quicker navigation overall (currently, this triggers a full SSR request)
  • Clicks on interactive elements (e.g. dropdown buttons) can be buffered until the app is loaded
    • While there will be a noticeable delay, but this is still a leg up on the current behaviour (just ignoring the events)

While working on #1760 it became clear that this feature won't be easy (or quick) to integrate

  • Elements need to have a unique id to support event replay. Currently this is not the case for a serious number of our components. For now, this branch can serve as a showcase of the kind & number of changes we would need to make to address this.
  • a.btn-style buttons won't work with event replay if defined with [routerLink]=""; they should use href="javascript:void(0)"; instead.
  • We should make sure that internal links are covered by event replay
  • Interactive buttons don't seem work consistently with event replay; this should be investigated further
  • Once implemented, testing all of these interactions will probably be quite involved as well

Instructions for Reviewers

TBD

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

ybnd added 11 commits August 4, 2022 18:53
preboot is triggered when the app first becomes stable, which caused the app to hang once CSR started
Tracked the issue down to initialization
  - GoogleAnalyticsService.addTrackingIdToPage()
  - AuthService.trackTokenExpiration()
SSR HTML inlines CSS into style.ng-transition elements. By default, Angular removes these as soon as CSR starts.
Because preboot buffers CSR off-screen, the SSR HTML remains visible during the transition - without CSS.

This workaround strips the ng-transition attribute from SSR styles so they aren't removed automatically
and subsequently removes these same styles once preboot is finished.
On the server, @slideSidebarPadding always resolved to 'expanded' because slideSidebarOver did not emit true
…imize-initial-page-load-by-leveraging-preboot
@lgtm-com
Copy link

lgtm-com bot commented Aug 11, 2022

This pull request introduces 1 alert when merging 818c142 into f6d2014 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

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

Successfully merging this pull request may close these issues.

2 participants