Skip to content

fix(uiGridAutoResize): Changed [0].clientHeight to gridUtil.elementHe… #6490

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

Merged
merged 3 commits into from
Dec 22, 2017
Merged

Conversation

tfilo
Copy link
Contributor

@tfilo tfilo commented Nov 30, 2017

I made change based on this issue: #6481
I changed $elm[0].clientHeight to gridUtil.elementHeight($elm) and $elm[0].clientHeight to gridUtil.elementWidth($elm).
I made another change too. I merged two watchers into one watchGroup. And I changed how uiGridCtrl.grid.refresh(); is called. Looks like it needs to be called inside

$scope.$apply(function(){
  uiGridCtrl.grid.refresh();
});

because without that it cause some problems when resizing grid with many columns. All tests still pass without problem.

function() {
return gridUtil.elementHeight($elm);
}], function(newValues, oldValues, scope) {
if (newValues.toString() !== oldValues.toString()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to use the toString prior to comparing?

Copy link
Contributor Author

@tfilo tfilo Dec 4, 2017

Choose a reason for hiding this comment

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

I used toString because it's simples method how to compare two arrays in javascript. Method toString is supported in all browsers. And I can't directly compare two arrays because newValues === oldValues will always return false because it will compare if instance of array is same. It will not check if content is the same. But when I convert arrays to string, if the content is same, I will get two identical strings and is easy to compare two strings. But I am open to any suggestions how to make it more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now when I think about it I changed it to angular.equals. It's better way in angular to compare arrays. Thanks for questioning.

if (newValues.toString() !== oldValues.toString()) {
uiGridCtrl.grid.gridWidth = newValues[0];
uiGridCtrl.grid.gridHeight = newValues[1];
setTimeout(function(){
Copy link
Member

Choose a reason for hiding this comment

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

Whys is a timeout needed here? Specially if the time is set to 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here they talk about it more. https://stackoverflow.com/questions/22346990/why-is-using-ifscope-phase-scope-apply-an-anti-pattern

If I called scope apply directly, it offten happens that I saw error in javascript console because apply was called in middle of another digest cycle. Which lead to error in javascript. So when I was looking to other solutions. This looked like best one.

@tfilo
Copy link
Contributor Author

tfilo commented Dec 6, 2017

After I consider all negatives of implementation with watcher I made other changes to prevent negative impact on performance. This latest implementation check width and height with watcher but instead of changing grid size directly it creates timeout. And if some another change occur before this timeout, it will set new timeout and cancel old one. So changes to grid will only happen after you stop resizing window.

So it should be even better with performance like original solution with interval 250ms. I know that there was even suggestions to replace watcher with event but I don't know how to do that. If you listen only event on windows resize, you will miss resize of grid parent container when some content of page change. For example hiding side menu. And I didn't find any solution how to make onResize based on parent container. So it is on you if you accept my solution. If someone can implement better solution I will glad to learn something new.

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.

2 participants