-
Notifications
You must be signed in to change notification settings - Fork 88
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 compact sidebar subtitle not visible #1767
Conversation
Signed-off-by: Raimund Schlüßler <[email protected]>
I wonder how we can distribute the fix to server. It is a problem in stable 21 and 20 (probably even in older versions). But they are still on vue-components 3.2.0 and 2.6.9, respectively. Would we need to release a vue components 3.2.1 and 2.6.10? Or should we directly commit a fix in stable 21 and 20 to workaround the issue in the components? |
@skjnldsv Six regression tests fail, with 0.013 difference. Could you have a look and upload new reference images if necessary? I think you did it the last time and the base images generated locally on my system differ from the ones the remote tests generate. |
yes |
We need to create the stable3 branch too |
Hm, the nextcloud-vue stable2 version is 2.9.0. Server stable 20 still lacks behind quite a bit. Is it realistic that server will get an update? |
depends on what the changelog between the two is? |
|
Seems good enough! |
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
/backport to stable2 |
The backport to stable2 failed. Please do this backport manually. |
@raimund-schluessler care to backport? 🙈 |
@skjnldsv Thanks for taking care of the cypress screenshots! I will have a look at the backport. |
@skjnldsv Backport is in #1789. By the way, updating the server CSS in #1537 broke some other stuff from the sidebar too.
The cut subtitle was coincidentally fixed by this PR #1767 here 🙈 But the text is still moving when enabling the edit mode (which is a nitpick only). I will still create an issue for the moving text and have a look: #1790. |
This fixes a CSS rule interference with a rule from server which lead to the sidebar subtitle not being visible in compact mode. This was originally reported here nextcloud/server#24138
Explanation:
This rule
from server took precedence over the
box-sizing: content-box;
from the components, which, in combination with the fixedheight: $desc-height;
, lead to the height of the description being wrong.I removed the fixed height (and the overwritten
border-box
), which should solve the issue. The alternative would be to setbox-sizing: content-box !important;
or to set a correctheight: $desc-height + 2*$desc-vertical-padding;
. But I think simply removing the height is cleaner.It might be that we need to commit new base images for the visual regression tests, because the sidebar height changes by 2px because of this fix (this is because the subtitle height is given as 22px, but since it is not enforced by actually setting the height of the subtitle, the subtitle is 24px high in reality).
Please have a look @skjnldsv.
Edit:
Before:
After: