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(LeftSidebar): revert temp styles and fix conversation styling #10818

Merged
merged 1 commit into from Nov 6, 2023

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Oct 31, 2023

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image
image image

🚧 Tasks

  • Code review
  • Visual review

🏁 Checklist

Comment on lines 167 to 173
adjustScrollerHeight() {
const scrollerContainer = this.$refs.scroller.$el.querySelector(
'.vue-recycle-scroller__item-wrapper'
)
const minHeight = this.conversations.length * this.CONVERSATION_ITEM_SIZE + 2
scrollerContainer.style.minHeight = minHeight + 'px'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not do this to fix the border...

  1. It updates only on conversation list update, but it doesn't work on height change, e.g. window resize. It can be fixed by window's resize event or resize observer, but it's not great for performance to do it just for a border.
  2. It directly mutates DOM nodes that are rendered and managed by Vue with Virtual DOM. We should never mutate Vue's DOM. It has many ways to break.
  3. It also searches for an element with a query selector, which is not a big issue but can be easily broken by a small layout change with a class rename.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ferdinand just suggested an alternative solution for such issues in NcCheckboxRadioSwitch: using outline-offset -2px. Then the outline it inside the element, and there is no issue with overflow. Maybe try using it here? Or fix it on the nextcloud/vue side...

Copy link
Contributor

Choose a reason for hiding this comment

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

Or to try to add a 2px footer in the virtual scroller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outline-offset is the optimal solution IMO but would rather change it in nextcloud/vue . If so, I will also add the overflow: hidden there too.

UPD: .. or not :p , let's keep it here as the outline might be off for other apps like Mail where there is a timestamp near the top-right edge.

@DorraJaouad DorraJaouad force-pushed the Techdebt/noid/adjust-listItem-styling branch from 9b0f903 to c4bd1d8 Compare October 31, 2023 16:06
Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
@nickvergessen
Copy link
Member

Replaced the milestone with 28 as the PR targets main.
If this is something for 27, we should backport it after merged to main

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Works, but there is only a native web-browser's outline. Not sure if it is expected.

image

@nickvergessen
Copy link
Member

Also the last item is cut off again?
Bildschirmfoto vom 2023-11-02 17-12-20

@DorraJaouad
Copy link
Contributor Author

Works, but there is only a native web-browser's outline. Not sure if it is expected.

image

Native outline seems to be on all elements not only list items.

Also the last item is cut off again?

That's the native outline being cut off (extra 1px I guess)

@DorraJaouad DorraJaouad merged commit 7489cce into main Nov 6, 2023
35 checks passed
@DorraJaouad DorraJaouad deleted the Techdebt/noid/adjust-listItem-styling branch November 6, 2023 08:59
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.

Conversation item border is cut of when tabbing
3 participants