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.

Sign up for GitHub

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

Jump to bottom

Fix tooltip appearance for long names in ConversationList #8605

Merged
merged 4 commits into from
Jan 31, 2023
Merged

Fix tooltip appearance for long names in ConversationList #8605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e989fc1
add length check after item has mounted
Antreesy Jan 26, 2023
847b70e
fix jest/node error
Antreesy Jan 26, 2023
9dafd29
change implementation to watch and ref
Antreesy Jan 27, 2023
47f2be1
update test to work with multiple promises
Antreesy Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
"@vue/vue2-jest": "^29.2.1",
"babel-loader-exclude-node-modules-except": "^1.2.1",
"babel-plugin-add-module-exports": "^1.0.4",
"flush-promises": "^1.0.2",
"jest": "^29.2.2",
"jest-environment-jsdom": "^29.3.1",
"jest-localstorage-mock": "^2.4.26",
Expand Down
6 changes: 5 additions & 1 deletion src/components/LeftSidebar/ConversationsList/Conversation.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ import { showSuccess, showError } from '@nextcloud/dialogs'

import Conversation from './Conversation.vue'

// Update after migration to @vue/test-utils v2.0.0
import flushPromises from 'flush-promises'

jest.mock('@nextcloud/dialogs', () => ({
showSuccess: jest.fn(),
showError: jest.fn(),
Expand Down Expand Up @@ -367,7 +370,8 @@ describe('Conversation.vue', () => {
const action = shallowMountAndGetAction('Leave conversation')
expect(action.exists()).toBe(true)

await action.find('button').trigger('click')
action.find('button').trigger('click')
await flushPromises()

expect(actionHandler).toHaveBeenCalledWith(expect.anything(), { token: TOKEN })
expect(showError).toHaveBeenCalledWith(expect.stringContaining('promote'))
Expand Down
20 changes: 19 additions & 1 deletion src/components/LeftSidebar/ConversationsList/Conversation.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
-->

<template>
<NcListItem :title="item.displayName"
<NcListItem ref="listItem"
:title="item.displayName"
:class="{'unread-mention-conversation': item.unreadMention}"
:anchor-id="`conversation_${item.token}`"
:actions-aria-label="t('spreed', 'Conversation actions')"
Expand Down Expand Up @@ -302,6 +303,23 @@ export default {
}
},
},

// TODO: move the implementation to @nextcloud-vue/NcListItem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting for discussion of nextcloud-libraries/nextcloud-vue#3687, if the same approach could be implemented there

watch: {
'item.displayName': {
immediate: true,
handler(value) {
this.$nextTick().then(() => {
const titleSpan = this.$refs.listItem?.$el?.querySelector('.line-one__title')

Comment on lines +313 to +314
Copy link
Contributor

Choose a reason for hiding this comment

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

If $refs.listItem is always defined, then all optional chainings ?. are redundant.

But if $refs.listItem maybe undefined, then it will fail anyway with a call of undefined. In such a case, it should have optional chaining on the function call querySelector?.('.line-one__title').

<NcListItem> is always rendered: there is no v-if on it or anything. IMO, we may drop checking with optional chaining here.

if (titleSpan && titleSpan.offsetWidth < titleSpan.scrollWidth) {
titleSpan.setAttribute('title', value)
Copy link
Member

Choose a reason for hiding this comment

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

All this just to set the title attribute? It would be a minor change in the nextcloud-vue lib to add this there without the need for watch and querySelector.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I know, it is not possible to detect if a text fit container or not without putting it in the container and looking at the result in DOM. watch + DOM access could be missed only if a title is always shown independently from title length.

But a change in nextcloud-vue may help to avoid manual mutating of Vue-rendered DOM.

}
})
},
},
},

methods: {
async copyLinkToConversation() {
try {
Expand Down