Merged
Conversation
45a3f0c to
4d1b335
Compare
…e postgresql queries and use in comments
55bf7c9 to
ee38c41
Compare
05a1571 to
0f2977b
Compare
ferblape
previously approved these changes
Apr 28, 2022
ahukkanen
requested changes
Apr 29, 2022
Contributor
ahukkanen
left a comment
There was a problem hiding this comment.
Very nice improvement, thanks a lot @entantoencuanto !
I just noticed one potential issue in the initial load when inspecting the network tab of the browser. I noticed that it is doing a duplicate AJAX request when the component initially loads. Please see the comments below.
decidim-comments/app/packs/src/decidim/comments/comments.component.js
Outdated
Show resolved
Hide resolved
decidim-comments/app/packs/src/decidim/comments/comments.component.js
Outdated
Show resolved
Hide resolved
ahukkanen
approved these changes
May 13, 2022
andreslucena
pushed a commit
that referenced
this pull request
May 19, 2022
* Send refresh comments request on comments component initialization * Load empty comments list in comments cell unless single_comment? * Add comments loading message * Fix lint errors * Don't instantiate counter when there's no textarea * Lint offenses * Don't call on null selector * Lint JS * Restore changes * No need to change hide class because now it's loaded automatically from ajax * Recover hide class removal on replies * Add a concern to deal with parent/child recursive resources using pure postgresql queries and use in comments * Ignore hidden comments when there are not comments at the same level or lower to show * Change comment_cell methods to use descendants recursive query * Fix test Currently the comments list is initially empty in the cell and they are loaded using AJAX * Take into account automatic translations in comments component * Define a method to fetch comments * Avoid duplicated requests in comments component Co-authored-by: Fernando Blat <fernando@blat.es>
andreslucena
pushed a commit
that referenced
this pull request
May 19, 2022
* Send refresh comments request on comments component initialization * Load empty comments list in comments cell unless single_comment? * Add comments loading message * Fix lint errors * Don't instantiate counter when there's no textarea * Lint offenses * Don't call on null selector * Lint JS * Restore changes * No need to change hide class because now it's loaded automatically from ajax * Recover hide class removal on replies * Add a concern to deal with parent/child recursive resources using pure postgresql queries and use in comments * Ignore hidden comments when there are not comments at the same level or lower to show * Change comment_cell methods to use descendants recursive query * Fix test Currently the comments list is initially empty in the cell and they are loaded using AJAX * Take into account automatic translations in comments component * Define a method to fetch comments * Avoid duplicated requests in comments component Co-authored-by: Fernando Blat <fernando@blat.es>
eliegaboriau
pushed a commit
to eliegaboriau/decidim
that referenced
this pull request
Oct 25, 2022
* Send refresh comments request on comments component initialization * Load empty comments list in comments cell unless single_comment? * Add comments loading message * Fix lint errors * Don't instantiate counter when there's no textarea * Lint offenses * Don't call on null selector * Lint JS * Restore changes * No need to change hide class because now it's loaded automatically from ajax * Recover hide class removal on replies * Add a concern to deal with parent/child recursive resources using pure postgresql queries and use in comments * Ignore hidden comments when there are not comments at the same level or lower to show * Change comment_cell methods to use descendants recursive query * Fix test Currently the comments list is initially empty in the cell and they are loaded using AJAX * Take into account automatic translations in comments component * Define a method to fetch comments * Avoid duplicated requests in comments component Co-authored-by: Fernando Blat <fernando@blat.es>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎩 What? Why?
This PR avoids loading all comments in the comments cell and loads them using javascript instead. In this way pages containing comments like the proposals or meetings show pages are loaded with similar times regardless of the number of comments they may contain.
Metrics
Data obtained from appsignal data of staging applicaction, with the show page of a meeting with 6 comments. Time in ms.
The time to load the comments partial (with a loading comments message initially) is significantly reduced.
Before optimization
Partials loading time:
After optimization
Partials loading time:
📌 Related Issues
Link your PR to an issue
Testing
Describe the best way to test or validate your PR.
📋 Checklist
🚨 Please review the guidelines for contributing to this repository.
docs/.📷 Screenshots
Please add screenshots of the changes you're proposing
