Feature/7600 - Scroll to newly created folder #9145
Conversation
pascalwengerter
force-pushed
the
feature/7600
branch
from
7d92c0b to
7cfb0e1
Compare
| @@ -66,12 +68,14 @@ export const useFileActionsCreateNewFolder = ({ | |||
| resource.indicators = getIndicators({ resource, ancestorMetaData: unref(ancestorMetaData) }) | |||
| } | |||
|
|
|||
| store.commit('Files/UPSERT_RESOURCE', resource) | |||
| await store.commit('Files/UPSERT_RESOURCE', resource) | |||
There was a problem hiding this comment.
My IDE suggests there's no difference when using await here, however the scrolling only happens if you put it there 🤷🏽
There was a problem hiding this comment.
I don't get that either... commit is a synchronous call / doesn't return a promise, so await makes no sense at all...
There was a problem hiding this comment.
https://stackoverflow.com/a/55263084
What you actually want is probably await nextTick()
There was a problem hiding this comment.
Done, thx for the hint!
| const fileListWrapperElement = document.getElementsByClassName('files-view-wrapper')[0] | ||
| fileListWrapperElement.scrollBy(0, -resourceElement.offsetHeight) | ||
| } | ||
| resourceElement.scrollIntoView({ behavior: 'smooth', block: 'center' }) |
There was a problem hiding this comment.
Not sure why all the other code was necessary tbh - scrolling to a resource and centering it on screen seems pretty straightforward, and if we need a "scroll to top" we should implement it explicitly?
There was a problem hiding this comment.
doesn't work with active pagination though. if you're on page 3 and a just created folder is sorted into page 17 you would need to navigate to that page before trying to scroll...
There was a problem hiding this comment.
Ah damn, I knew there was something I was missing (because otherwise that issue would've been resolved long ago already I suppose)
There was a problem hiding this comment.
fine to split it into two issues btw. for this PR: assume that using scrollToResource is sufficient and declare it not checking pagination as bug. Then solve using pagination in scrollToResource as a separate PR.
There was a problem hiding this comment.
Not sure why all the other code was necessary tbh - scrolling to a resource and centering it on screen seems pretty straightforward, and if we need a "scroll to top" we should implement it explicitly?
It was there for the keyboard navigation. Now when navigating via keyboard, it always immediately scrolls to the next resource. Before, it only scrolled when the resource was outside of the viewport. Both works, but I actually like the latter because it does not feel so "floaty". Maybe we can handling things a bit differently when working with keyboard navigation? Pass a param like forceScroll to the method which is false in case of keyboard navigation?
|
Adding some opinion: scrolling to resource would also make sense when uploading a resource and even when creating new file that opens in new tab |
pascalwengerter
force-pushed
the
feature/7600
branch
from
7cfb0e1 to
d8bcc3e
Compare
pascalwengerter
force-pushed
the
feature/7600
branch
from
d8bcc3e to
f5a5231
Compare
pascalwengerter
force-pushed
the
feature/7600
branch
from
f5a5231 to
5e09c8b
Compare
packages/web-app-files/src/composables/keyboardActions/useKeyboardActionsSearchTable.ts
Show resolved
Hide resolved
packages/web-app-files/src/composables/keyboardActions/useKeyboardActionsSearchTable.ts
Show resolved
Hide resolved
|
SonarCloud Quality Gate failed.
|
|
Sonarcloud failure is unrelated to my changes? 🤔 |
| @@ -46,6 +50,8 @@ describe('useFileActionsCreateNewFolder', () => { | |||
| title: '"myfolder" was created successfully' | |||
| }) | |||
| ) | |||
|
|
|||
| // expect scrolltoresource to have been called | |||
There was a problem hiding this comment.
Something for a follow-up I propose 🙂
|
@pascalwengerter @dschmidt it doesn't scroll when new folder is on another page |
* Fix #7600 && #7601 * Fix useFileActionsCreateNewFolder.spec.ts * Fix unit tests --------- Co-authored-by: Dominik Schmidt <[email protected]>
I've created #9494 to keep track of this. It's a bit more complex to achieve that. |
* Fix #7600 && #7601 * Fix useFileActionsCreateNewFolder.spec.ts * Fix unit tests --------- Co-authored-by: Dominik Schmidt <[email protected]>
* Fix #7600 && #7601 * Fix useFileActionsCreateNewFolder.spec.ts * Fix unit tests --------- Co-authored-by: Dominik Schmidt <[email protected]>
Description
Upstreams a CERN commit by @elizavetaRa which originates from before some major refactorings. It also reduces the
scrollToResourcearguments from a full resource where only theidwould be used to a string/number.Related Issue