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

Fix missing expand/collapse tree icon when refreshing page #2681

Merged
merged 2 commits into from
Jun 18, 2018
Merged

Fix missing expand/collapse tree icon when refreshing page #2681

merged 2 commits into from
Jun 18, 2018

Conversation

rasmusjp
Copy link
Member

@rasmusjp rasmusjp commented Jun 10, 2018

Prerequisites

Description

This fixes expand/collapse tree icons not being shown in the navigation tree when the page is refreshed

Before:
before

After:
after

@emmaburstow
Copy link
Contributor

Thanks for this one too @rasmusjp! We'll look over it shortly.

Emma, Community Pull Request team

Copy link
Contributor

@emmaburstow emmaburstow left a comment

Choose a reason for hiding this comment

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

Hi @rasmusjp

This is ace for two reasons. One, you've made ten lines of code into one and it does the job nicely. Two, this had been bothering me too ;)

I've tested it and am happy to approve. Thanks for your work,

Emma

@madsrasmussen
Copy link
Contributor

It is absolutely amazing we are getting closer to fix this issue because it is a little bit annoying.

One thing we have to keep in mind before merging: The reason why we moved the logic from the template to the link function was part of a performance improvement. If you have many nodes in the tree a lot of watchers can bring Angular to its knees. The PR will bring back an "ng-style" which might be okay but it will add to the numbers of watchers on the tree.

@rasmusjp
Copy link
Member Author

@madsrasmussen Sure, I'll gladly look into solving this another way. But looking at the history ng-style haven't been used before. If there might be a performance problem with ng-style wouldn't it also happen for ng-class and ng-show which also are used in the template?

From what I can see the problem occurs in the first place because loadNodeChildren() calls removeChildNodes() which sets hasChildren to false.

So maybe adding a way to trigger a "node refresh" when loadNodeChildren has finished would be better.

Please let me know if I should look into doing that instead

This fixes the expand/collapse icon not being show when a tree
is autoexpanded, e.g. when refreshing the page
@rasmusjp
Copy link
Member Author

rasmusjp commented Jun 12, 2018

I've updated the PR, so now instead of adding ng-style we just call updateNodeData on the node instead, which updates the dom node. This is the same thing that's being done when the node is reloaded. Not sure how I've missed that function in the first place.

This shouldn't have any impact on the performance on large trees and the result should be the same.

@emmaburstow , would you have a look again and verify it still works as expected?

@emmaburstow
Copy link
Contributor

hi @rasmusjp!

Sure, I'll have a look tonight :) Thanks for that

Emma

Also there's no need to send the node to the function
since it's the same node, so better save some compute
power there too
@emmaburstow
Copy link
Contributor

Hi @rasmusjp - all good here at my end! @madsrasmussen are you happy with the changes?

@madsrasmussen
Copy link
Contributor

Awesome! If all tests are OK then I think we are good to go 🎉

@nul800sebastiaan nul800sebastiaan merged commit 3e12908 into umbraco:dev-v7 Jun 18, 2018
@nul800sebastiaan
Copy link
Member

Great stuff! Thanks @rasmusjp 👍

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.

4 participants