Client side rendering of the pages admin #923
Conversation
|
I've prepared a stage to preview changes. Open stage or view logs. |
|
|
||
| page_list.each_with_index do |page, i| | ||
| if page.parent_id != path.last[:id] | ||
| if path.map{ |o| o[:id] }.include?(page.parent_id) # Lower level |
There was a problem hiding this comment.
Style/SpaceBeforeBlockBraces: Space missing to the left of {.
|
Wow, this is amazing. I'll nitpick around a bit though. |
|
Please resend this pull request to master. We can back port it to 3.2 if everything works as expected. Thanks |
|
I really appreciate your contribution. I will have a look later on the implementation. One note on your commit messages. Please use imperative mood. Also a lots of the commit messages are only related to former commits and should quashed into these commits instead of being separate commits. But this can be addressed later on. Again, this is very much appreciated. Can't wait to try this. |
| @@ -25,6 +27,59 @@ def index | |||
| end | |||
| end | |||
|
|
|||
| # Returns all pages as a tree from the root given by the id parameter | |||
| # | |||
| def tree | |||
There was a problem hiding this comment.
I think we should move this into the api namespace. What do you think @mamhoff?
|
@jorrizza this is great stuff. I like the way this is going! Thank you very much |
| self.items = $(".sitemap_page", '#sitemap') | ||
| self._observe() | ||
| Alchemy.PageSorter.init() if self.sorting | ||
| Alchemy.pleaseWaitOverlay(false) |
There was a problem hiding this comment.
I don't like to have this for this feature. I would love to have a small spinner besides the root page or something. The please wait overlay is just too much for this and confusing for the users, because the learned, this only happens, when switching pages. Also, if a user just clicked the page tree tab ad then again the overlay appears, might confuse them.
There was a problem hiding this comment.
We have Alchemy.Spinner, that's build on top of Spin.js. That should help.
There was a problem hiding this comment.
Good point! I'll see if I can make it easier for our users that way.
|
@tvdeyen I'll do that as soon as I'm able. Apart from it not being based off of master, only the handlebars asset is the remaining issue so far. Right? |
|
@jorrizza I will have a thorough look tomorrow. Thanks so far for your awesome work |
|
@jorrizza I just saw, that you target |
|
@tvdeyen No problem. I'll do that as soon as I can. My priority for now was to push a fix live as soon as possible, hence it's based off of |
|
We've given this a spin, having encountered similar slow behaviour from our admin front page, and yeah, can confirm the 10x speed increase for us too. So needless to say - we are very keen to get this change added into a version of alchemy we can use. Can I ask what version this change would be scheduled for? I note the desire to have this PR rebased against master (4.0) which makes me then wonder what the timescale for that release might be? Is there a potential for a 3.3 release, for instance? We are keen to see this happen, and happy to put some time and legwork into getting this over the line, but would like to understand the best path to getting a non-forked version with this improvement in it. E.g. not so keen on a scenario of spending a chunk of time getting this bedded into master, only to make 3-6 months for the release, especially as master is not regarded as production stable. |
|
@phantomwhale thanks for testing this feature. We definitely could carve a 3.3 release for that. I also think that this is a mayor improvement. I still need some time to test the UI, cause I'm still not satisfied with the "always-fold-the-whole-tree" approach. I would love to see only sub branches being re-rendered on unfolding a sub-tree, instead of the whole tree. But, if that would be a lot of work, we could also improve this later on. Thoughts @mamhoff @robinboening @moritzps ? |
|
Thanks for all the feedback, @tvdeyen! All of your feedback has been applied I think. How would you like to see the commit history? Do you prefer a single big, merged commit or do I just have to weed out the small correctional ones? I see the milestone has been removed. So this won't make it in the 3.x release after all? |
|
Wow! Thanks @jorrizza 🍰 Will have a look into it asap. I removed the milestone so we don't block the 3.3 release. But if this here is ready, we will release it with 3.3. If not we will release a 3.4 shortly after. Thanks for all your work!! 🎈 |
|
@jorrizza and the commit history as small and meaningful as possible. Mostly by removing all the correctional ones. If one big commit is "the right thing" (in terms of completely working change set), then this will do. And please add a CHANGELOG entry. Thanks! |
|
@tvdeyen There's not CHANGELOG.md file yet in the 3.2-stable branch. Should I also rebase this branch to master? |
|
Yes. Please target this pull request to master. Thanks. |
|
An already linked page should be selected if I open the link overlay: Link page 831
Page 831 should be selected
In order to achieve this, we need to add a callback to the page tree renderer, that calls the Tricky one... Sorry 😿 |
|
That should do it. Thanks for the wonderful screenshotted feedback. It's really helpful. |
|
Wow, that was fast! Unfortunately we need to change this once again. Sorry Scenario: You have an external link already set. If you open the dialog again we now always render the page tree before we switch the tab to the selected one. That's not preferred for "External Links" I recorded a screencast to show what I mean: We need to change the flow here:
Sorry, for leading you in the wrong direction with |
|
This commit solves the work flow when editing an external link, but leaves the page tree to load in the background. In my opinion this should not be a problem. |
|
👏 works great! Thanks @jorrizza for all your work Now please:
Then this is ready to be shipped |
|
I can't retarget this pull request, can I? Does that mean I have to make a new pull request and just reference this one? In that case I'll make a new branch with just one or two commits. |
|
@jorrizza it seems you can't change branches anymore on Github.... So, then yes. You need to close this one and create a new one targeting |
This is an attempt to make the
/admin/pages/page render much more quickly by using client side template rendering with Handlebars. The speed increase is at least tenfold when compared to the current 3.2 branch.I'm sure some bits and pieces can be made a bit less ugly. I'm not a Coffee/JavaScript hero.