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 tooltip appearance for long names in ConversationList #8605

Conversation

Antreesy
Copy link
Contributor

Signed-off-by: Maksim Sukharev antreesy.web@gmail.com

Fix #7254

Due to using the NcListItem component with title locked for modifications, we don't have opportunities to add v-tooltip here.
So, HTML-attribute title was applied for long names to show tooltip via native browser tools, when name is ellipsed.

!IMPORTANT
Current realisation works only if name is ellipsed initially (action button appears on hover and crops name even more)

πŸ–ΌοΈ Screenshots

image

🚧 TODO

  • Cross-browser testing (working on Google)
  • Design review
  • Code review

🏁 Checklist

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

We don't use inline if's in Nextcloud :)

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@@ -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

@Antreesy Antreesy marked this pull request as draft January 27, 2023 13:59
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.

Everything is good for me now. Only one small comment about optional chaining.

Comment on lines +313 to +314
const titleSpan = this.$refs.listItem?.$el?.querySelector('.line-one__title')

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.

@Antreesy Antreesy marked this pull request as ready for review January 30, 2023 12:57
@Antreesy
Copy link
Contributor Author

Antreesy commented Jan 30, 2023

this.$nextTick() prevent 'errors with notification when a new moderator is required before leaving' test from src/components/LeftSidebar/ConversationsList/Conversation.spec.js to pass.

Added flush-promises for Jest to work with multiple promises. Dependency could be removed after migration to Vue 3 and @vue/test-utils v2.0.0 (also added TODO for this).

Thanks to @ShGKme for the suggestion

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@ShGKme
Copy link
Contributor

ShGKme commented Jan 30, 2023

this.$nextTick() prevent 'errors with notification when a new moderator is required before leaving' test from src/components/LeftSidebar/ConversationsList/Conversation.spec.js to pass.

Added flush-promises for Jest to work with multiple promises. Dependency could be removed after migration to Vue 3 and @vue/test-utils v2.0.0 (also added TODO for this).

Thanks to @ShGKme for the suggestion

For the record.

What happens in the test:

  1. Click on the "Leave conversation" button, which has some async handler
  2. await trigger('click') (= await nextTick())
  3. Check that the result of the handler is correct
const action = shallowMountAndGetAction('Leave conversation')

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

expect(actionHandler).toHaveBeenCalled()

What was not correct:

A promise of the async handler is not related to the nextTick's promise. Waiting for async in Vue reactivity doesn't guarantee some async method (handler) to be resolved.

What is correct:

Wait for the handler's promise to be resolved. It's not easy to do. But waiting for all promises to be resolved works fine, and that is a common practice. flushPromises does it.

Why it worked previously and failed after the update in this PR:

The async handler was kind of the only async process in the component. Waiting for the nextTick's promise was enough because its handler stayed later in the microtasks queue.

  1. Mount component
  2. Click on the button - the async handler is executed
  3. Mocked executor in async hander finished immediately - add the promise's resolve handler to the microtasks queue (X)
  4. await nextTick - add Vue scheduler's promise's resolve handler to the microtasks queue (Y)
  5. Go to the microtasks queue. X is done
  6. Go to the microtasks queue. Y is done
  7. βœ… Check that X is done

Calling nextTick early on the component's creation breaks it.

  1. Mount component
  2. await nextTick - add Vue scheduler's promise's resolve handler to the microtasks queue (Y)
  3. Click on the button - the handler is executed
  4. Mocked executor in async hander finished immediately - add the promise's resolve handler to the microtasks queue (X)
  5. Go to the microtasks queue. Y - done
  6. ❌ Check that X is done
  7. Go to the microtasks queue. X - done. Too late...

Adding waiting for literally any promise would work too.

await action.find('button').trigger('click')
await Promise.resolve() // Even this is enough to wait for the end of microtasks queue in this case

@nickvergessen
Copy link
Member

/backport to stable25

const titleSpan = this.$refs.listItem?.$el?.querySelector('.line-one__title')

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.

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.

When adding a user with a very long name to a conversation group, all information is not displayed
5 participants