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

Defer Diff Loading on Changes Tab #15887

Merged
merged 9 commits into from Mar 28, 2024

Conversation

ncounter
Copy link
Contributor

@ncounter ncounter commented Mar 25, 2024

Applying the common pattern: load the static/text part of the page within the main page request, send an async call right after the page load to fetch the heavy content of the page while a loading message is visible.

Test

Visit https://obs-reviewlab.opensuse.org/ncounter-async-loading-changes/request/show/1/changes

Before: the page is loaded fully sync - spot the loading icon at the top of the browser tab

before-async-loading

After: the page is quickly loaded in the header and the rest but the content, that starts loading async

after-async-loading

@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Mar 25, 2024
@ncounter ncounter force-pushed the async-loading-changes branch 2 times, most recently from 17d6b94 to 64e05eb Compare March 26, 2024 00:07
@ncounter ncounter added the review-app Apply this label if you want a review app started label Mar 26, 2024
@obs-bot
Copy link
Collaborator

obs-bot commented Mar 26, 2024

Review app will appear here: http://obs-reviewlab.opensuse.org/ncounter-async-loading-changes

@ncounter ncounter force-pushed the async-loading-changes branch 4 times, most recently from 8d70634 to 50504eb Compare March 26, 2024 08:07
@ncounter ncounter marked this pull request as ready for review March 26, 2024 11:12
@ncounter ncounter force-pushed the async-loading-changes branch 3 times, most recently from 6a0d973 to 6a77f58 Compare March 27, 2024 08:24
@ncounter ncounter mentioned this pull request Mar 27, 2024
@@ -0,0 +1,12 @@
- if diff_to_superseded_id
You're reviewing changes from
= link_to("superseded request ##{diff_to_superseded_id}.", request_show_path(number: diff_to_superseded_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

@ncounter we should stick to the bs_request number here. id != number. Usually its the same, but its still two different things, and could lead to problems if we exchange them.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah its just the naming, you are using the number. but we should stay consistent with the naming here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, it's only a variable name to the loop to keep the queryString parameter alive. The parameter is populated in set_diff_to_superseded_id.

I'm going to rename it. Thinking about it: before it was diff_to_superseded, now it is diff_to_superseded_id, I'm going for diff_to_superseded_number but I also wonder if I could get rid of the diff_to prefix and finalize it to superseded_number only 🤔 The fact that we use it for the diff is a detail, we could use it for something else too. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah its confusing, diff_to_superseded doesnt contain any diff. Its simply the superseding request object. I would also just call it this way. Maybe simply superseding_request and superseding_request_number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed and decided offline, the renaming change will happen in a separate PR due to the fact the changes involves much more than the scope of this PR.

#15906

@ncounter ncounter merged commit ad7ff62 into openSUSE:master Mar 28, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app review-app Apply this label if you want a review app started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants