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

Loading indicator active analyzers #2855

Merged
merged 39 commits into from
Aug 29, 2023

Conversation

Annoraaq
Copy link
Collaborator

Adresses all 5 points of #2799

We will keep the activeAnalyses and the finished analyses/analyzerResults in the store. That way we don't need the EventBus anymore.

As soon as there are new activeAnalyses the AnalyzerResults components starts polling the active analyses. If they change, the Analyzer results are loaded from the backend again and stored in the store.

In the AnalyzerList component, a different icon is shown if the analyzer has been run for ALL currently selected timelines.

@jkppr jkppr self-assigned this Jul 21, 2023
@Annoraaq Annoraaq requested a review from jkppr July 21, 2023 15:08
Copy link
Collaborator

@jkppr jkppr left a comment

Choose a reason for hiding this comment

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

First: Great solution with using the store for the analysis sessions. The whole implementation feels more solid, reactive and stable during my tests.

I ran into some smaller nits and bugs, please take a look below.

@Annoraaq Annoraaq requested a review from jkppr August 4, 2023 14:23
Copy link
Collaborator

@jkppr jkppr left a comment

Choose a reason for hiding this comment

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

Awesome work over all. Just three small comments to be discussed. Afterwards we are ready to merge from my point of view. Thanks a lot for the efforts!

@Annoraaq Annoraaq requested a review from jkppr August 25, 2023 13:23
@jkppr jkppr merged commit bdea046 into google:master Aug 29, 2023
20 checks passed
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.

3 participants