Repository navigation
feat: keyboard navigation to timeline - #17798
Conversation
github-actions
Bot
added
documentation
midzelis
removed
documentation
github-actions
Bot
added
documentation
midzelis
removed
documentation
danieldietzler
left a comment
danieldietzler
left a comment
There was a problem hiding this comment.
Iterators definitely made this a lot more elegant.
| findAssetById(assetDescriptor: AssetDescriptor) { | ||
| return this.assetsIterator().find((asset) => asset.id === assetDescriptor.id); | ||
| } |
There was a problem hiding this comment.
I don't really know how easy that'd be with your current data model, but just a thought: We could potentially get data information for that asset id through the API. Would we be able to more efficiently grab the correct bucket (or even a small set of buckets, I don't really care) to then only linearly search within that?
There was a problem hiding this comment.
Any actual api call will far outweigh the gains in efficiency. That said, sometimes we do know the date, and we can create an optimized one that uses binary search in those cases. But that would be a future PR.
There was a problem hiding this comment.
Maybe I'm misunderstanding then. This won't possibly iterate over 1M assets?
There was a problem hiding this comment.
This specific method is defined on AssetBucket, so its limited to a single bucket. The bucket was already loaded. This is used when navigating to a specific asset given only an asset id (from the URL). The entire asset is retrieved (getAssetInfo) - then the bucket for that asset is found (searching for yearMonth), and then, within that bucket, it searches for the asset. However, there are a few places where we already have asset instead of just id, and this search can be eliminated. I'll update with more efficient versions
There was a problem hiding this comment.
On second thought, we don't actually have the real asset. When the keynav keystroke is pressed, it looks up the current asset using a data-asset attribute. A LRU cache could help, but honestly, the lookups are pretty fast. We could add a LRU cache later, but it would make things a bit more complex. That should be done if profiling determines its necessary, and in a diff PR.
There was a problem hiding this comment.
Yeah I don't really think we need to employ caching here. The timeline is already very fast.
* feat: improve focus * feat: keyboard nav * feat: improve focus * typo * test * fix test * lint * bad merge * lint * inadvertent * lint * fix: flappy e2e test * bad merge and fix tests * use modulus in loop * tests * react to modal dialog refactor * regression due to deferLayout * Review comments * Re-use change-date instead of new component * bad merge * Review comments * rework moveFocus * lint * Fix outline * use Date * Finish up removing/reducing date parsing * lint * title * strings * Rework dates, rework earlier/later algorithm * bad merge * fix tests * Fix race in scroll comp * consolidate scroll methods * Review comments * console.log * Edge cases in scroll compensation * edge case, optimizations * review comments * lint * lint * More edge cases * lint --------- Co-authored-by: mertalev <101130780+mertalev@users.noreply.github.com> Co-authored-by: Alex <alex.tran1502@gmail.com>
Description
Note: this PR is stacked with PR #17796
This PR adds the ability to navigate timeline using keyboard shortcuts. Using d/D for next/previous day, m/M for month, y/Y for year. And 'G' to open a 'goto' dialog, to go to any date.