Skip to content

Fixed 'animate' issue. - #1392

Closed
marttop wants to merge 1 commit into
gridstack:developfrom
marttop:develop
Closed

marttop wants to merge 1 commit into
gridstack:developfrom
marttop:develop

Conversation

@marttop

@marttop marttop commented Oct 2, 2020

Copy link
Copy Markdown

Description

#937 Fixing this issue by not recreating the stylesheet.

#1391 I created another pull request (It's for sure not the proper way to do but I still struggle a lot with git) and followed @adumesny's advices :

  • I don't check anymore for existing styles because I could prevent _max from being reset every time
    For cellHeigh() and margin(), on method _updateStyles I added a boolean parameter 'doForceUpdate' witch is set to true only when called from cellHeight() or margin(). In that case I reset the stylesheet.
  • I tried my best to respect your coding style as you asked.
  • I tried to remove the _initStyles method and add it to _updateStyles but kept on breaking the behaviour so I let you do it... It seems I don't know enough on the component to do so.

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (yarn test)
  • Extended the README / documentation, if necessary

@adumesny

adumesny commented Oct 2, 2020

Copy link
Copy Markdown
Member

thanks for taking a second look. I think I want to re-write this all thing rather than try to patch what's there... but I need to understand all corner cases first. We also should delete the style when destroy() happens (which we don't) so we need a separate removeStyle() method that will also reset the max info structure...

Probably best to let me do it at this point. I'm tied this weekend but might get to it middle of next week.

@marttop

marttop commented Oct 2, 2020

Copy link
Copy Markdown
Author

I understand that you want to rewrite it, but just in case, this at least fixes the bug temporarily.

@adumesny

adumesny commented Oct 4, 2020

Copy link
Copy Markdown
Member

thank you for fix suggestion! I ended up re-writing that code instead #1395 AND default to animate since SO much nicer (I even didn't know about it).

@adumesny adumesny closed this Oct 4, 2020
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