Feature/ switch explorer rendering to use react-virtualized #2182
Conversation
akinsho
changed the title
akinsho
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2182 +/- ##
==========================================
- Coverage 44.51% 44.48% -0.04%
==========================================
Files 351 351
Lines 14302 14318 +16
Branches 1865 1863 -2
==========================================
+ Hits 6367 6369 +2
- Misses 7709 7723 +14
Partials 226 226
Continue to review full report at Codecov.
|
akinsho
changed the title
|
|
||
| switch (true) { | ||
| case renameInProgress: | ||
| return 35 |
There was a problem hiding this comment.
Hmm, how do we determine these sizes? I'm wondering if they are dependent on the ui.fontSize setting? I'm just worried about a bug where different font sizes cause rendering quirks.
There was a problem hiding this comment.
I actually fiddled with the sizes here and tbh the css with virtualized involved is more or less opaque to me but I didn't notice a big difference using the values which were trial and error as a result of that process, I've tweaked it however to use ui.fontSize to derive the size in case
| </div> | ||
| <ExplorerContainer className="explorer"> | ||
| <AutoSizer> | ||
| {({ height, width }) => ( |
There was a problem hiding this comment.
Nice! Thanks for wiring this all up 👍
Ah ya, I hit this too - didn't get a chance to log an issue, but it definitely was a problem! Thanks so much for fixing it!
Awesome!! 💯 Thanks for going to those lengths to fix it. This will be much better long-term too, especially for large directories with tons of files - we only have to render whats on the screen.
Can't wait! 😄 Overall the change looks great! I had one small concern around how we calculate the row heights - I'm not sure how it works with different font sizes, so I wanted to see what you thought there. Thanks @Akin909 ! |
…x-flickering-scroll-explorer
…x-flickering-scroll-explorer
|
@bryphe made the changes you suggested re row height 👍 hopefully this is good to go EDIT: Seems to be an issue with caching on appveyor.. not sure how to resolve that but they don't seem related to these changes. |
| const isSelected = id === selectedId | ||
|
|
||
| const PADDING = 10 | ||
| const fontSize = parseInt(this.props.fontSize, 10) |
There was a problem hiding this comment.
Hmm, I wonder if this handles the various cases we allow for fontsize? For example, it looks like 11px, 11pt, 11em would all get handled the same way here, but they would actually have very different pixel sizes. I think what we'd want instead is the measured font size, or some more deterministic way to get the size of the cells.
|
|
||
| switch (true) { | ||
| case renameInProgress: | ||
| return rowSize * 1.5 |
There was a problem hiding this comment.
I'm a bit concerned about these magic numbers here - 1.5 and 2.8. If someone changes the styling of the node or the creation, they might not realize they need to update these here, and get difficult to find bugs. Along the lines of the above comment - is there a more deterministic way we can get these sizes?
…x-flickering-scroll-explorer
add helper for quickly adding types for styled component oni props
|
@CrossR could you give this PR a try/review especially re creating and renaming and moving files, I've kind of lost perspective on it think its a big improvement (re. the stutter) and havent noted any bugs today but previously noted a resizing issue which seems to have resolved itself..... |
akinsho
changed the title
|
Hey @Akin909, it looks like this PR has a branch conflict now. Can you take a look? |
|
I'm still getting some weirdness with the scroll sticking to the bottom. If I scroll past the bottom of the menu, the bar sticks to the bottom for the entire time now. There is also some jumping still where the bar vanishes for a bit and then reappears after letting go of the scroll button. Another thing I've noticed is that on inserting items sometimes the spacing is off.
Its up to you if you want to sort them in this PR or in a follow up, neither are as bad as the flickering, so I'd understand getting this in and sorting the bugs later(in a smaller more contained PR). Let me know and if you decide on that, I can approve the PR. No point doing that now since there is a merge conflict. |
use more thorough method for checking if props changed simplify sidebar items components
switch to invalidating the cache if yanking, pasting etc occur
|
@CrossR thanks for having another look was reeeeeally stuck with the bug you noticed with the spacing I've switched the strategy to a more brute force one instead of only trying to update the affected nodes when something like a rename etc happens I instead now trigger a redraw/update of all the visible nodes so should avoid random ones here or there not being measured properly. Re holding the button and the bar disappearing when is that happening? I haven't noted it since I switched to having the bar stick on the bottom although, one of the limitations of using virtualized and the cellMeasurer component from the docs is
So without finding a faster library or a different implementation button mashing could always lead to this although I there might be little ways to optimise this or hide it at least. Although the library authors have been talking about a new version for a while now so we'll see. Anyway if you get a moment can you try it again to see if the weird spacing bug is still there(locally I havent noted it anymore), I never found a consistent way to reproduce it (your repro doesnt always bring it on nor have I found it to consistently reproduce) |
Seems to still happen when I scroll rapidly in the Oni repo: I can't seem to repro the other bug now though, and I tried 10-15 times whereas before it was happening maybe every 2/3 times I tried. |
|
@CrossR thanks for the GIF that helps a lot 💯 , re that issue it relates to the quote above re performance and the strategy were using basically the library cant keep up with the holding the button As far as the docs say there is no clear way round it I'm hoping future releases improve performance and we get a fix for free if not down the line we might have to switch to a different strategy I have no idea what, the main value of the library is for scrolling v. long lists not quite what were doing which is scrolling to indexes very fast using keyboard input |
|
Since it seems the really annoying bug is no longer reproducible (I haven't encountered it again locally) I'd like to close this PR off and finally delete the branch 😆 |
|
@CrossR thanks for the approval (again 👍) |
An issue I noticed with the explorer was that on reaching the lowest boundaries of the visible files/folders the scroll position would flicker on each move downwards making it hard/impossible to move to files which are out of view.
Having attempted to use/tweak the
scrollIfNeededfunction with no luck I've refactored the explorer to usereact-virtualizedinstead which I believe was mentioned at some point was an eventual change we wanted to make, which has the added bonus of providing methods that can be used to control scrolling to selected elements.This change also sets up the
searchInExplorerfunctionality I'm planning by allowing for programmatically focusing search of a particular element.Also due to ongoing difficulties identifying dom nodes as
styled-componentsuses hashed classnames I've also added the typescript version ofbabel-plugin-styled-componentswhich generates more readable classnames e.g.NodeWrapper-asdfsfs2324rather thansc-dfaffr3242Todo: