-
Notifications
You must be signed in to change notification settings - Fork 112
Rework outline / table of contents #8033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6ad97d1 to
122f405
Compare
3ddd4ee to
16dc1e0
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
f831d49 to
a4e17e3
Compare
marcoambrosini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice @mejo- :)
One point from my side:
- I would trigger the outline on hover, not click
- Once hovered, I would provide a pin button in the same location where the
xbutton is right now. Once the pin button is pressed, we can then display thexbutton to unpin
jancborchardt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @marcoambrosini about triggering it on hover. And when leaving the area, it should close again.
Instead of automatically staying open and needing to click a "close" button, we can have a "pin" icon that can be toggled, as proposed before. :)
Thanks for the feedback. I was unsure about that one, but now that I implemented it, I agree it's much better. I updated the desktop screencast, please have a look |
541a93b to
b974a67
Compare
marcoambrosini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
max-nextcloud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reviewed the first three commits for now.
All in all looks great! I have a bit of nitpicking never the less 😉
| headings() { | ||
| this.$nextTick(() => { | ||
| this.setupIntersectionObserver() | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no need to teardown and rebuild the intersection observer here. Should be enough to update the observed entries.
Something along these lines:
headings(newHeadings, oldHeadings) {
this.updateIntersectionObserver(newHeadings, oldHeadings)
},
...
updateIntersectionObserver(newHeadings, oldHeadings)
const newIds = new Set(newHeadings.map((h) => h.id))
const oldIds = new Set(oldHeadings.map((h) => h.id))
const added = Set.difference(newIds, oldIds)
const removed = Set.difference(oldIds, newIds)
removed.forEach((id) => this.unobserve(id))
this.$nextTick(() => {
added.forEach((id) => this.observe(id))
})
},
observe(id) {
const el = document.getElementById(id)
if (el) {
this.intersectionObserver.observe(el)
}
},
.... unobserve like above - but i am not sure if that's even needed given that the element will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also make the watcher robust against overwriting the headings array with the same content - it would just do nothing as no ids were added or removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I figured out a way to even do without the set difference. I'll give it a try and let you know.
| const editorWidthDesktop = 'min(80ch, 100%)' | ||
| const editorWidthDesktopEnhanced = 'min(80ch, (100% - 2 * 40px))' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could express this in css - as in using some sibling selector.
| // as it may render other vue components such as preview toggle | ||
| // which breaks the context of the setup function. | ||
| this.setContent(this.content, { addToHistory: false }) | ||
| this.updateHeadings() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this even needed given that the composable also seems to initialize the headings?
max-nextcloud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise no blockers from my side.
jancborchardt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! Could be further improved with some details like a quick animation for sliding in and out, as well as not giving a whole row for just the pinning icon, but that can be done in a follow-up. :)
Signed-off-by: Jonas <jonas@freesources.org>
Fixes: #7228 Signed-off-by: Jonas <jonas@freesources.org>
Fixes: #3586 Fixes: #5379 Fixes: #6876 Fixes: #7522 Fixes: nextcloud/collectives#1624 Fixes: nextcloud/collectives#1715 Fixes: nextcloud/collectives#1777 Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Fixes: #6693 Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
…reactive Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Gets rid of some code duplication. Signed-off-by: Jonas <jonas@freesources.org>
We don't display floating buttons and table of contents in rich workspace or in plain text editor, so no need to provide whitespace for them. Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
b974a67 to
21fdd27
Compare
Also remove some dead code. Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
21fdd27 to
97a855d
Compare
|
Psalm failures are unrelated. |
Fixes small editor window on mobile with empty or short content. Fixes regression from #8033. Signed-off-by: Jonas <jonas@freesources.org>
📝 Summary
See #7741 (comment)
Screenshots desktop
Screenshots mobile
Screencast desktop
Screencast_Desktop.mp4
Screencast mobile
recording2.mp4
🚧 TODO
useTableOfContentsflag fromuseEditorFlagsis always false inuseEditorWidthwhen loading Collectives🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)