#3504655: UI enhancement
Closes #3504655
Merge request reports
Activity
added 43 commits
-
37ae01f9...13820f2e - 41 commits from branch
project:2.0.x - 9d3ff706 - Merging 2.0.x
- 61904d5f - Don't scroll on initial load
-
37ae01f9...13820f2e - 41 commits from branch
added 1 commit
- fb843b0d - Fixes some test but we might revert this commit once we get a better solution
370 position: relative; 370 371 flex: 1 1 0; 372 transition: opacity 0.3s ease-in-out; 373 } 374 375 .pb-layout__main.pb-project__loading { 376 opacity: 0.5; 377 } 378 379 .pb-layout__main .pb-projects__loading-overlay { 380 position: absolute; 381 top: 0; 382 left: 0; 383 width: 100%; 384 height: 100%; 385 background: rgba(255, 255, 255, 0.6); - Comment on lines +380 to +385
This assumes that there is only one project browser on the page, which is no longer a safe assumption to make. We support mini-browsers, you could therefore have multiple project browsers on the page.
This needs to be adjusted so that it only covers the project browser that's doing the loading, rather than the entire screen...unless I'm misreading this CSS and that's what it's already doing. But if that's the case, then this needs a comment to clarify that only the project browser that's doing the loading is going to be obscured.
changed this line in version 16 of the diff
added 7 commits
-
fb843b0d...4b49d356 - 5 commits from branch
project:2.0.x - 7c70c3a6 - Merging 2.0.x
- ea3f2024 - Test fixing trial
-
fb843b0d...4b49d356 - 5 commits from branch
added 2 commits
added 3 commits
-
2a6d5bc1...66b308ef - 2 commits from branch
project:2.0.x - ca869228 - Merge branch '2.0.x' into 3504655-ui-enhancement
-
2a6d5bc1...66b308ef - 2 commits from branch
39 let hasMounted = false; 40 let hasLoadedOnce = false; 41 42 onMount(() => { 43 hasMounted = true; 44 }); 45 46 afterUpdate(() => { 47 if (hasMounted && hasLoadedOnce && mainContainer && !loading) { 48 mainContainer.scrollIntoView({ behavior: 'smooth', block: 'start' }); 49 } 50 if (!loading) { 51 // Mark as loaded after the first load cycle 52 hasLoadedOnce = true; 53 } 54 }); - Comment on lines +38 to +54
This needs more comments.
In particular, I don't understand why we have so many flags being checked in
afterUpdate(). I need a comment that explains why:- Why do we need
hasMounted? I wouldn't thinkafterUpdate()would ever be invoked until the component was mounted. Am I wrong about that? - Why do we need
hasLoadedOnce? Presumably it's so that we don't do this scrolling behavior on the first load; it's so that we only do it if a user changes a filter or goes to a different page. But in that case, I don't think we'd needhasLoadedOnce; instead, we could usefilters.subscribe()andpage.subscribe()to set a flag that we check.hasLoadedOncefeels a bit like a hack. - Why are we checking for the existence of
mainContainer? Is there ever a possibility that it will NOT have a value by the time thatafterUpdate()runs? - The
!loadingcheck feels a bit awkward. Why not just callmainContainer.scrollIntoView()in theload()function, rather than inafterUpdate()?
- Why do we need
I'll try to give the answers point wise in the same order:-
- You’re correct that
afterUpdate()does not run before the component is mounted. However,afterUpdate()runs on every update after the component is mounted, and this includes updates that happen immediately after mounting.This was the reason why i added that flag. - This flag is used to prevent automatic scrolling on the first load.
- Svelte’s reactive statements and lifecycle functions don’t guarantee that an element bound with
bind:this={mainContainer}is immediately available.While debugging this issue i was able to get the main container null so that was the reason why i added(it mainly happened when we didn't have the ability to prevent scroll on initial load). - This is something that felt a bit odd to me as well.As it was added by @omkar-pd and as far as i understood it was mainly to complete the rendering cycle.
- You’re correct that
-
However,
afterUpdate()runs on every update after the component is mounted, and this includes updates that happen immediately after mounting.This was the reason why i added that flag.So if afterUpdate() runs after the component has mounted, can
hasMountedever befalse?This flag is used to prevent automatic scrolling on the first load.
This makes sense, and I can't really find anything to complain about with it. It would benefit from a comment.
Svelte’s reactive statements and lifecycle functions don’t guarantee that an element bound with
bind:this={mainContainer}is immediately available.While debugging this issue i was able to get the main container null
mainContainerto benull?as far as i understood it was mainly to complete the rendering cycle.
If we don't understand it and it feels strange, let's remove it and see if anything breaks. I'd kind of rather we just had a helper function, like
scrollToTopOfComponent()or something, that we explicitly called fromload(). changed this line in version 16 of the diff
467 467 ], in_order: TRUE); 468 468 469 469 // Select 'Z-A' sorting order. 470 $this->sortBy('z_a'); 470 $this->sortBy('z_a', TRUE); changed this line in version 16 of the diff
761 761 $assert_session = $this->assertSession(); 762 762 763 763 $this->drupalGet('admin/modules/browse/project_browser_test_mock'); 764 $this->assertPageHasText('Results'); changed this line in version 16 of the diff
added 27 commits
-
ca869228...b3ca3c38 - 11 commits from branch
project:2.0.x - b3ca3c38...ce2bbec2 - 6 earlier commits
- 0645a157 - Test fixing trial
- 8c4424b1 - Some more changes
- f00f4208 - Temp fix for tests
- a532165c - Phpcs fixes
- c0a4a5ec - Phpcs fixes
- e3dbeb5d - Reverted unwanted changes
- f79f4f24 - Fixed testPaginationWithFilters
- d1a73d9d - Fixed phpcs issues
- 40b96b8c - Reverted all changes
- 12e6dc2e - Changes done in ProjectBrowser.svelte
Toggle commit list-
ca869228...b3ca3c38 - 11 commits from branch
1113 1113 padding: 5px; 1114 1114 font-size: 0.79rem; 1115 1115 } 1116 1117 [data-project-browser-instance-id] { 1118 position: relative; 1119 } 1120 1121 [data-project-browser-instance-id] .pb-projects__loading-overlay { 1122 position: absolute; 1123 z-index: 5; changed this line in version 21 of the diff