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

Bump @nextcloud/vue to 3.1.0 #4473

Merged
merged 2 commits into from
Oct 29, 2020
Merged

Bump @nextcloud/vue to 3.1.0 #4473

merged 2 commits into from
Oct 29, 2020

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Oct 27, 2020

This brings in:

Todos

Tests

I did a quick test to see if all still works:

  • scrolling in left sidebar still works fine
  • scrolling in participant list works fine
  • sending (typing)/receiving messages still works, with emojis
  • remove hacks
  • check settings panel for regression
  • verify chat scrolling with lots of messages when conversation in sidebar
  • verify regular chat scrolling

Obsoletes #4457

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

please also cleanup old css hacks

/* Force scroll bars in tabs content instead of in whole sidebar. */
::v-deep .app-sidebar-tabs {
height: calc(100% - 80px);
&__content {
overflow: hidden;
section {
height: 100%;
overflow-y: auto;
}
}
}

@PVince81
Copy link
Member Author

I had a quick look at #4195 and I don't see any difference before and after the Vue update, so I'd say it's not a blocker. The PR in question needs further work and might require further changes in nextcloud-vue, for which we can do future releases + version bumps.

@PVince81
Copy link
Member Author

PVince81 commented Oct 27, 2020

@skjnldsv when I remove the whole block you mentioned (specifically, the overflow:hidden), there's a double scrollbar again.

I compared the sidebar before and after the vue update, removing or adding the hack back, I saw no difference in behavior.
So I'm not sure how the new tab scrollbar approach is supposed to help in this situation.

@skjnldsv
Copy link
Member

@skjnldsv when I remove the whole block you mentioned (specifically, the overflow:hidden), there's a double scrollbar again.

Yeah, it's because of

margin-top: 10px;
padding-top: 0;
outline: none; /* remove the weird border that appears on focus */

You need to remove this as well.
Please do not remove the outline! This is an accessibility feature and is here on purpose.
Generally speaking you should not override components styling. If it's here, it's here on purpose and for all apps.

@PVince81
Copy link
Member Author

Indeed. so this will reopen #2946 and we need to find a different solution for it, maybe rework the tab contents layout. Ideally the scroll container should start below the field instead of having the field floating and overlapping. This means having a scroll container even deeper inside the tab content and not using the tab content's own scroller.

@skjnldsv
Copy link
Member

Ideally the scroll container should start below the field instead of having the field floating and overlapping.

No need, just put the padding on the form, and it will have the background under it?
Position sticky is exactly why it was designed for :)

@skjnldsv
Copy link
Member

skjnldsv commented Oct 27, 2020

Also works perfectly fine for me by defaults
Peek 27-10-2020 10-36

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member Author

@skjnldsv easier said than done. I'll need some CSS (or even Vue) trickery to remove the padding on the parent ".app-sidebar-tabs__content" to add it back to the input field. But only do that when the participants tab is selected.

@PVince81
Copy link
Member Author

never mind, was looking at the wrong element...

@PVince81 PVince81 force-pushed the update-nextcloud-vue-3.1.0 branch from 9642617 to 5dfdffb Compare October 28, 2020 14:12
@PVince81 PVince81 force-pushed the update-nextcloud-vue-3.1.0 branch from 5dfdffb to 51b5361 Compare October 28, 2020 14:58
@PVince81
Copy link
Member Author

I've verified the scrolling with lots of messages in the files app sidebar and it worked fine! (had to use #4291 for testing)

If CI is green I think we can move forward with this PR.

Remove obsolete hacks for the right sidebar.
Adjusted padding of search field to hide participants behind when
scrolling, the former approach did not work any more.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
Co-authored-by: John Molakvoæ <skjnldsv@users.noreply.github.com>
@PVince81 PVince81 force-pushed the update-nextcloud-vue-3.1.0 branch from 51b5361 to 4c7b6a0 Compare October 28, 2020 15:58
@PVince81 PVince81 merged commit 05b3d39 into master Oct 29, 2020
@PVince81 PVince81 deleted the update-nextcloud-vue-3.1.0 branch October 29, 2020 08:22
@PVince81
Copy link
Member Author

I forgot to verify the sidebar chat during a call (I only checked in the files app).
It has a double scrollbar now, raised here #4492

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

Successfully merging this pull request may close these issues.

2 participants