Skip to content
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

Position menububble relative to content-wrapper #1903

Merged
merged 4 commits into from
Nov 5, 2021

Conversation

azul
Copy link
Contributor

@azul azul commented Oct 20, 2021

Summary

Add a position: relative to content-wrapper
to fix misalignment of the menububble.

The menububble has an absolute position
which is calculated by tiptap
based on the position of the highlighted text.

It is styled with left and bottom attributes.
These are relative to the position of the .content-wrapper.

@azul azul requested review from juliusknorr and mejo- October 20, 2021 09:21
@azul
Copy link
Contributor Author

azul commented Oct 20, 2021

Before:
Bildschirmfoto von 2021-10-20 10-59-01

After:
Bildschirmfoto von 2021-10-20 10-59-21

After scrolling:
Bildschirmfoto von 2021-10-20 10-59-34

@juliusknorr
Copy link
Member

The only issue I see now would be that the menububble no longer overlays the menubar on the very first line of the rich workspace.

image

This seems to be due to the scroll overflow of the content-wrapper and due to the fact that we don't have the upper margin to keep the rich workspace compact.

@juliusknorr
Copy link
Member

Might be one of the things that tiptap has resolved on the v2 release as the new bubble menu component seems to dynamically align at the top or bottom of the selected text, depending on the available space: https://tiptap.dev/examples/menus

@juliusknorr
Copy link
Member

Actually we can make use of ueberdosis/tiptap#459 it seems so we'd just need to either use top or bottom depending on if the menububble fits the available space to the menu bar.

@juliusknorr juliusknorr added 2. developing bug Something isn't working labels Oct 27, 2021
@azul azul force-pushed the azul/fix-1677-menububble-position branch 2 times, most recently from ef6636a to 750fe80 Compare October 27, 2021 14:46
@azul
Copy link
Contributor Author

azul commented Oct 27, 2021

in the viewer

second line

Bildschirmfoto von 2021-10-27 16-46-13

first line

Bildschirmfoto von 2021-10-27 16-46-03

in the workspace in files

first line

Bildschirmfoto von 2021-10-27 16-45-45

second line - slightly too high

Bildschirmfoto von 2021-10-27 16-45-32

I suspect the size of the content-wrapper is changed somehow in the workspace making the bottom alignment get a little higher.
Turns out it's not just a little but a lot once i start scrolling content.

@azul azul force-pushed the azul/fix-1677-menububble-position branch 2 times, most recently from 1a8f340 to 0b19101 Compare October 27, 2021 15:45
azul and others added 4 commits November 5, 2021 16:42
Add a `position: relative` to `content-wrapper`
to fix misalignment of the menububble.

The menububble has an absolute position
which is calculated by tiptap
based on the position of the highlighted text.

It is styled with `left` and `bottom` attributes.
These are relative to the position of the `.content-wrapper`.

Signed-off-by: Azul <azul@riseup.net>
For the rest of the text we keep the position above.

Signed-off-by: Azul <azul@riseup.net>
In rich workspaces scrolling still made the menu-bubble render at the wrong place.

The element that the absolute position of the menu-bubble is relative to needs to grow
with the text so tiptap can position the menu-bubble correctly.

Now we also do not need the margin bottom anymore.

Signed-off-by: Azul <azul@riseup.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the azul/fix-1677-menububble-position branch from 0b19101 to 8dddfc7 Compare November 5, 2021 15:47
@juliusknorr
Copy link
Member

Took the liberty to push a lint fix and rebase ;)

@juliusknorr juliusknorr merged commit 09517c0 into master Nov 5, 2021
@juliusknorr juliusknorr deleted the azul/fix-1677-menububble-position branch November 5, 2021 15:59
@skjnldsv skjnldsv mentioned this pull request Nov 8, 2021
23 tasks
@juliusknorr
Copy link
Member

/backport to stable22

@juliusknorr
Copy link
Member

/backport to stable21

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@azul azul restored the azul/fix-1677-menububble-position branch November 19, 2021 10:08
@azul
Copy link
Contributor Author

azul commented Nov 19, 2021

/backport to stable21

@azul
Copy link
Contributor Author

azul commented Nov 19, 2021

/backport to stable22

@backportbot-nextcloud
Copy link

The backport to stable21 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable22 failed. Please do this backport manually.

azul added a commit that referenced this pull request Jan 4, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.
azul added a commit that referenced this pull request Jan 4, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
azul added a commit that referenced this pull request Jan 5, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
azul added a commit that referenced this pull request Jan 5, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
azul added a commit that referenced this pull request Jan 10, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
@max-nextcloud max-nextcloud added backported successfully backported and removed backport-request backported successfully backported labels Jan 13, 2022
julien-nc pushed a commit that referenced this pull request Jan 13, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
julien-nc pushed a commit that referenced this pull request Jan 13, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
max-nextcloud pushed a commit that referenced this pull request Jan 18, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
max-nextcloud pushed a commit that referenced this pull request Jan 19, 2022
* roll back parts of #1903 that broke it again.
* Calculate top of menububble based on scrollHeight of content-wrapper.
* Always display menububble below selected line.
  This will make it less likely to conflict with the menubar
  or mobile copy and paste toolbars.
* Add cypress tests for the workspace.

Signed-off-by: Azul <azul@riseup.net>
@max-nextcloud max-nextcloud deleted the azul/fix-1677-menububble-position branch February 17, 2022 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants