Skip to content

Conversation

@luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Mar 27, 2023

๐Ÿ“ Summary

๐Ÿ–ผ๏ธ Screenshots

๐Ÿš๏ธ Before ๐Ÿก After
image outline-scroll.webm

๐Ÿšง TODO

  • ...

๐Ÿ Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@luka-nextcloud luka-nextcloud self-assigned this Mar 27, 2023
@luka-nextcloud luka-nextcloud requested a review from mejo- March 27, 2023 13:50
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for looking into this. Could we get rid of the horizontal scrollbar in cases where it's not necessary?

@cypress
Copy link

cypress bot commented Mar 27, 2023

1 failed and 2 flaky tests on run #9370 โ†—๏ธŽ

1 141 1 0 Flakiness 2

Details:

fix: scroll for outline
Project: Text Commit: 81ad97db9d
Status: Failed Duration: 04:47 ๐Ÿ’ก
Started: Apr 12, 2023 12:34 PM Ended: Apr 12, 2023 12:38 PM
Failedย  cypress/e2e/sync.spec.js โ€ข 1 failed test

View Output Video

Test Artifacts
Sync > recovers from a lost connection Output Screenshots
Flakinessย  sync.spec.js โ€ข 1 flaky test

View Output Video

Test Artifacts
Sync > saves the actual file and document state Output Screenshots
Flakinessย  share.spec.js โ€ข 1 flaky test

View Output Video

Test Artifacts
Open test.md in viewer > Share a file with download disabled shows an error Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@luka-nextcloud
Copy link
Contributor Author

@mejo- I got rid of the horizontal scrollbar.

@luka-nextcloud
Copy link
Contributor Author

@juliushaertl @mejo- Please check again.

@luka-nextcloud luka-nextcloud force-pushed the bug/scroll-outline-issue branch from bb1942d to a50a0dc Compare March 28, 2023 16:08
@luka-nextcloud luka-nextcloud requested a review from mejo- March 28, 2023 18:15
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear @luka-nextcloud,

I see several problems with the PR as it is right now:

  • max-height: calc(100% - 100px) works only in Viewer, not in Collectives (where we have an extra title bar of 60px height).
  • padding-bottom: 100px in .editor-outline seems like the wrong approach to me.
  • .editor--outline__header is not sticky.
  • Width of the .editor--toc__item items still grows bigger than the container width.

Here's the local changes that fixed those issues for me, maybe you can take a look and apply them if the seem sensible to you:

diff --git a/src/components/Editor/EditorOutline.vue b/src/components/Editor/EditorOutline.vue
index 36915b224..373913634 100644
--- a/src/components/Editor/EditorOutline.vue
+++ b/src/components/Editor/EditorOutline.vue
@@ -55,10 +55,11 @@ export default {
 <style lang="scss" scoped>
 .editor--outline {
        width:  300px;
-       padding: 0 10px 100px 10px;
+       padding: 0 10px 10px 10px;
        position: fixed;
        overflow: auto;
-       max-height: calc(100% - 100px);
+       // 204px = 50px nc header + 60px collectives titlebar + 44px menubar + 50px bottom margin
+       max-height: calc(100% - 204px);
 
        &-mobile {
                box-shadow: 8px 0 17px -19px var(--color-box-shadow);
@@ -69,6 +70,9 @@ export default {
        &__header {
                margin: 0;
                position: sticky;
+               top: 0;
+               z-index: 1;
+               background-color: var(--color-main-background);
                padding: 0.6em 0.6em 0.6em 0;
                display: flex;
                align-items: center;
diff --git a/src/components/Editor/TableOfContents.vue b/src/components/Editor/TableOfContents.vue
index 064ba42bc..3f860ac9d 100644
--- a/src/components/Editor/TableOfContents.vue
+++ b/src/components/Editor/TableOfContents.vue
@@ -90,6 +90,7 @@ export default {
                overflow: hidden;
                white-space: nowrap;
                animation: initialPadding calc(var(--animation-duration) * 2);
+               width: calc(100% - var(--padding-left));
 
                a:hover {
                        color: var(--color-primary-hover);

@mejo-
Copy link
Member

mejo- commented Mar 29, 2023

Please note that one problem remains even with my changes: the max-height of the outline container is not really dynamic. If Text gets used somewhere with even more vertical offset to the window top, it might not work. But the only other option I see is to set max-height dynamically using Javascript by calculating the vertical offset. Maybe @juliushaertl or @max-nextcloud have a better idea?

@juliusknorr
Copy link
Member

juliusknorr commented Apr 5, 2023

This seems trickier than expected. I was just thinking if it would make sense to move the table of content component outside of the .editor-content-wrapper to be able to align it with the top element of text but this could become even more complex than having a resize observer on the #editor-container and adapt it dynamically to that height. One further case where this will not work though is with the current scroll behaviour within deck as we use a scroll container outside of text there.

I'm unsure about how we best proceed here, so maybe we discuss this in our call next week.

Signed-off-by: Luka Trovic <[email protected]>
@mejo- mejo- force-pushed the bug/scroll-outline-issue branch from 06d0047 to b741f92 Compare April 12, 2023 11:38
@mejo-
Copy link
Member

mejo- commented Apr 12, 2023

After discussing this again with @juliushaertl, we agreed to merge the PR as it is for now. At least it fixes the issue for Text in Viewer, standalone and in Collectives.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luka-nextcloud I took the liberty to rebase and squash the commits.

@mejo- mejo- force-pushed the bug/scroll-outline-issue branch from 462c208 to e5f57db Compare April 12, 2023 12:02
@mejo-
Copy link
Member

mejo- commented Apr 12, 2023

/compile

Signed-off-by: nextcloud-command <[email protected]>
@juliusknorr juliusknorr added bug Something isn't working 4. to release labels Apr 12, 2023
@juliusknorr juliusknorr added this to the Nextcloud 27 milestone Apr 12, 2023
@mejo- mejo- merged commit 5ea361d into main Apr 12, 2023
@delete-merged-branch delete-merged-branch bot deleted the bug/scroll-outline-issue branch April 12, 2023 12:43
@mejo-
Copy link
Member

mejo- commented Apr 12, 2023

/backport b741f92 to stable26

@mejo-
Copy link
Member

mejo- commented Apr 12, 2023

/backport b741f92 to stable25

@mejo-
Copy link
Member

mejo- commented Apr 12, 2023

/backport /b741f9237e4b5b1335fd02d33b333e366703e57e,e5f57db008e73f40a7b00db5ff57bf2dcc7753cc to stable26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release backported successfully backported bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scrolling in heading outline

5 participants