Cellnav rewrite and speed improvements #2084
Merged
Conversation
This file contains 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
The function was previously only allowing one parameter. Now the args just get passed straight on to $log.debug
prevScrollLeft and prevScrollTop were being set improperly in the adjustScroll* methods. Neither were accounting for the viewport, causing the value to be too large. Now it only sets the value if the scrollLeft or scrollTop argument is not provided, and it includes the viewport in the calculation. Also minor fixes within cellNav
The filter term watches were all firing whether or not there was a change to the values!
applyHide() was running on all GRID_SCROLL events, whether or not the menu was visible. This was causing excess $digest() loops.
renderContainers with no canvasWidth were having their heights calculated. If their columns were wrapping around it would mean too large of a height on that renderContainer, and thus would be propogated to ALL the other containers (i.e. pinned containers).
When navigating beyond the left/right edges of grid around to the other side, the grid was losing focus which prevented further navigation because any keydown events were being sent to <body> (the new activeElement). This fix causes the renderContainer to be REFOCUSED after a GRID_SCROLL event in the case that the activeElement is document.body. We are assuming that when scrolling within the grid, an element within the grid should have focus.
When hitting the ESC key in a cell while editing it, the cell was losing focus when it should have been retained. This fix accounts for the changes to using actual CSS classes instead of relying on :focus.
c0bra
added a commit
that referenced
this pull request
Nov 13, 2014
Cellnav rewrite and speed improvements
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a large set of changes that focuses on reverting how we track elements within the the grid's row repeater. For the cellNav feature we changed to tracking by row.uid, which was causing us to recycle DOM elements at a very quick pace, resulting in thrashing the browser.
Going back to
track by $indexrequired rewriting the cellNav feature so that it no longer relies on "true" browserfocus. but fakes it by handling click events and tracking the history of how we navigate cells. These changes to cellNav ALSO requiresdmany changes inside the edit feature, as it was implicitly relying onfocusas well. We still do depend on focused elements, but only in the sense that we need to have them capture keyboard events.There are also several speed improvements and fixes that were discovered in the course of this change, e.g. watches that shouldn't have been firing, calculations running when they shouldn't have and causing race conditions, etc.