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

Improve Activity Log page request lifecycle #3016

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Conversation

balanza
Copy link
Member

@balanza balanza commented Sep 24, 2024

Description

Refine how the Activity log page handles the API request

  • define the HTTP request lifecycle in lib/api/request
  • show an empty view initially
  • show an error view in case an error occurred
  • add a spinner when loading data

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Interesting the introduction of the MainView and the modeling of the lifecycle of an API request. Curious about the possibility to be reused elsewhere.

However tests for ActivityLogOverview and ActivityLogPage are failing and need to be adjusted.

setLoading(false);
});
// defer the loading state to avoid flickering
const tid = setTimeout(() => setActivityLogRequest(request.loading()), 500);
Copy link
Member

Choose a reason for hiding this comment

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

question: how can the flickering be reproduced?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, it is something visually annoying that has no functional effect. Maybe it only applies to local executions, when the latency is near zero.

Copy link
Member

Choose a reason for hiding this comment

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

Alright thanks for clarifying.

@balanza balanza force-pushed the activity-log-spinner branch 2 times, most recently from 12a996f to 24a75cc Compare September 26, 2024 13:35
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Besides the conflict to be resolved (my bad 😄 ) LGTM! Thanks!

@balanza balanza force-pushed the activity-log-spinner branch from 24a75cc to cb379d6 Compare September 27, 2024 07:45
@balanza balanza enabled auto-merge (squash) September 27, 2024 07:45
@balanza balanza merged commit 0b8ad99 into main Sep 27, 2024
27 checks passed
@balanza balanza deleted the activity-log-spinner branch September 27, 2024 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants