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

Load release list only when needed #2113

Merged
merged 2 commits into from
May 3, 2023

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Apr 16, 2023

It was something we discussed some time ago. Instead of loading the release list all the time, with this PR it's only loaded when the menu is opened. It should greatly reduce the page size on crates with a lot of releases.

Original issue: #1999

cc @Nemo157 @jsha

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Apr 16, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the ajax-release-list branch 2 times, most recently from ab1da03 to 85cd7ca Compare April 19, 2023 13:33
@GuillaumeGomez
Copy link
Member Author

I finished porting the code so now JS generates the same thing as jinja used to. The problem now is that some tests relied on the DOM content to check some things, which isn't possible anymore. We could however have the failing tests ported to some "front-end testing"like what #1698 proposes.

@Nemo157
Copy link
Member

Nemo157 commented Apr 20, 2023

Rather than doing client-side rendering of json, we could just serve a partial-page of HTML that gets inserted in the DOM, that might be easier to migrate the existing tests to too.

@GuillaumeGomez
Copy link
Member Author

That would indeed make it much simpler to maintain and we could directly re-use the existing template. 👍

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Apr 20, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the ajax-release-list branch 3 times, most recently from d1c2a20 to 0525f8f Compare April 21, 2023 13:34
@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Apr 21, 2023
@GuillaumeGomez
Copy link
Member Author

I applied suggestions: this path now returns HTML which is directly inserted into the DOM, allowing to greatly simplify the code and also minimize the impact on tests.

@GuillaumeGomez
Copy link
Member Author

@Nemo157: Anything else that needs to be done here?

@GuillaumeGomez
Copy link
Member Author

Applied suggestions.

@GuillaumeGomez GuillaumeGomez merged commit 1b46084 into rust-lang:master May 3, 2023
@GuillaumeGomez GuillaumeGomez deleted the ajax-release-list branch May 3, 2023 18:14
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels May 3, 2023
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants