Skip to content
This repository has been archived by the owner on Aug 28, 2023. It is now read-only.

Preload indicator chart static config #1093

Merged
merged 3 commits into from
Apr 6, 2018
Merged

Conversation

rmartz
Copy link
Contributor

@rmartz rmartz commented Apr 4, 2018

Overview

In the indicator chart component, each configuration controller renders once its data has loaded, producing a disorganized effect as components individually pop in. Because each controller handles its own loading, using a collective guard against all of them appears to be infeasible.

Instead, this leverages the caching behavior of the services, issuing an "eager" request on app load so the cache is already hot when needed by a user.

Eager loading is performed in the AppModule component structured on this StackOverflow answer.

Demo

indicator chart open mov

Testing Instructions

  • Log out and refresh

  • Log in

  • Navigate to the indicators page

  • Open an indicator chart

    • The control elements should all load simultaneously
  • Refresh

  • Open an indicator chart

    • The control elements should still load simultaneously
  • Is the "[Unreleased]" section of the CHANGELOG.md updated with any changes that might complicate the next release? Consider adding links to any related PRs if additional context is required, but avoid only including the link in the CHANGELOG, with no additional description.

Connects #998

@rmartz rmartz requested a review from fungjj92 April 4, 2018 17:06
@rmartz rmartz changed the title Preload indicator chart static config [WIP] Preload indicator chart static config Apr 4, 2018
@rmartz rmartz force-pushed the feature/preload-static-config branch from 87d690b to a8afab9 Compare April 4, 2018 18:48
@rmartz rmartz changed the title [WIP] Preload indicator chart static config Preload indicator chart static config Apr 4, 2018
@rmartz
Copy link
Contributor Author

rmartz commented Apr 4, 2018

Fixed the linter error, was worried there was a problem causing tests to fail locally with an odd error, but all appears to be good

@rmartz rmartz force-pushed the feature/preload-static-config branch from a8afab9 to ea960eb Compare April 5, 2018 15:45
@rmartz
Copy link
Contributor Author

rmartz commented Apr 5, 2018

Rebased and added CHANGELOG entry.

Copy link
Contributor

@fungjj92 fungjj92 left a comment

Choose a reason for hiding this comment

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

Yooooo 👏 Nice!! Works great, clean implementation. Just one typo in a comment. Sorry for review delay~

private scenarioService: ScenarioService,
private modelService: ClimateModelService) {
this.userService.currentUser.first().subscribe(() => {
// Issue a eager request for indicator static configuration data so it's already cached if
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "an" eager

@fungjj92 fungjj92 assigned rmartz and unassigned fungjj92 Apr 6, 2018
@rmartz rmartz force-pushed the feature/preload-static-config branch from ea960eb to 9223c66 Compare April 6, 2018 14:07
@rmartz
Copy link
Contributor Author

rmartz commented Apr 6, 2018

Thanks! Fixed the grammatical error, merging now

@rmartz rmartz merged commit 16143c1 into develop Apr 6, 2018
@rmartz rmartz deleted the feature/preload-static-config branch April 6, 2018 14:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants