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

Fix link menu positioning #1045

Merged
merged 2 commits into from
Oct 8, 2020
Merged

Fix link menu positioning #1045

merged 2 commits into from
Oct 8, 2020

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Sep 16, 2020

Summary

Fixes some styling issues with the menu bubble positioning and giving it a fixed height of 44px since it otherwise would change height when switching to the link input.

I'm not entirely sure about the 44px height since it looks kind of huge compared to the rest, but for now I picked this since those are buttons and we have a minimum click target of 44px

Before

image

After

image

Notes for backporting

⚠️ When backporting we need to specify an additional fixed width for this since tiptap doesn't handle resizing the menu bubble properly. With Nextcloud < 20 the single button is smaller than the input which caused that the input could be cut of due do missing repositioning. A width of 180px seems to be fine as a fix for older versions for this.

@juliusknorr juliusknorr added bug Something isn't working 3. to review labels Sep 16, 2020
@juliusknorr
Copy link
Member Author

/backport to stable19

@juliusknorr
Copy link
Member Author

/backport to stable18

Copy link
Contributor

@azul azul left a comment

Choose a reason for hiding this comment

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

Looks good to me. I had a look both at the code and tested it (rebased on current master) locally. The height change makes sense to me.

I was not able to reproduce the alignment issue discussed in #600 - with current master.
Maybe this has already been fixed. The changes in here do not seem to target it anyway. Just the branch name made me read the bug description and see if it had any effect.

edit: looking more closely it does seem to have the effect that the left edge of the popup does not get cut off anymore in the files workspace. Tested with chromium and firefox on linux.

@juliusknorr
Copy link
Member Author

I was not able to reproduce the alignment issue discussed in #600 - with current master.

Yeah #600 was somehow mitigated on master/stable20 since with the two buttons the initial width of the button container was higher, therefore the initial position calculations work better when switching to the input for links.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good! Would be even better with white background for the menu and the divider in color-border, but we can also do that separately. :)

@jancborchardt
Copy link
Member

@juliushaertl needs rebase, but then let’s get it in? :)

Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Copy link
Member Author

Rebased and also added a design fix as discussed in our call:
image

@juliusknorr juliusknorr merged commit 454821f into master Oct 8, 2020
@juliusknorr juliusknorr deleted the bugfix/600 branch October 8, 2020 14:33
@backportbot-nextcloud
Copy link

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

@backportbot-nextcloud
Copy link

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

@MorrisJobke
Copy link
Member

🙄

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.

4 participants