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

feat: reduce items rendering in left sidebar search results #14669

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: reduce items rendering in left sidebar search results #14669

wants to merge 6 commits into from
+352 −207

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Mar 17, 2025 •
edited
Loading

☑️ Resolves

Rendering went from n items rendered out of n (100%) to 15-30 items out of n items.

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After

🚧 Tasks

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@DorraJaouad DorraJaouad force-pushed the fix/left-sidebar-search branch 3 times, most recently from 5d74dbe to a30186e Compare March 19, 2025 22:34
@DorraJaouad DorraJaouad force-pushed the fix/left-sidebar-search branch from 822e53b to 21a55e2 Compare March 20, 2025 15:57
@DorraJaouad DorraJaouad changed the title feat: implement recycle scroller to left sidebar search results feat: reduce items rendering in left sidebar search results Mar 20, 2025
@DorraJaouad DorraJaouad self-assigned this Mar 20, 2025
@DorraJaouad DorraJaouad requested review from Antreesy and ShGKme March 20, 2025 16:09
@DorraJaouad DorraJaouad added this to the 🪺 Next Major (32) milestone Mar 20, 2025
@DorraJaouad DorraJaouad marked this pull request as ready for review March 20, 2025 16:09
@DorraJaouad DorraJaouad force-pushed the fix/left-sidebar-search branch from 21a55e2 to 8818ae1 Compare March 20, 2025 16:36
@DorraJaouad DorraJaouad force-pushed the fix/left-sidebar-search branch from 8818ae1 to e3a876f Compare March 21, 2025 11:18
Antreesy
Antreesy reviewed Mar 24, 2025
View reviewed changes
src/components/LeftSidebar/LeftSidebar.vue
<Hint v-else-if="contactsLoading" :hint="t('spreed', 'Loading …')" />
</ul>
<SearchConversationsResults v-else
ref="searchResults"
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be considered in list-ref prop for arrow navigation

src/components/LeftSidebar/SearchConversationsResults/SearchConversationsResults.vue
Comment on lines +321 to +324
<template v-if="sourcesWithoutResults">
<NcAppNavigationCaption :name="sourcesWithoutResultsList" />
<Hint :hint="t('spreed', 'No search results')" />
</template>
Copy link
Contributor

Choose a reason for hiding this comment

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

That IMO should be added as last items in searchResultsVirtual array

ShGKme
ShGKme reviewed Mar 25, 2025
View reviewed changes
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.

Some stylistic comments

src/components/LeftSidebar/SearchConversationsResults/SearchConversationsResults.vue
Comment on lines +91 to +94
// Add "New Conversation" option if allowed
if (props.canStartConversations) {
virtualList.push({ type: 'listItem', id: 'new_conversation', name: props.searchText, subname: t('spreed', 'New private conversation') })
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be before the search results?

src/components/LeftSidebar/SearchConversationsResults/SearchConversationsResults.vue
Comment on lines +44 to +54
const searchResultsConversationList = computed(() => {
if (props.searchText !== '' || props.isFocused) {
const lowerSearchText = props.searchText.toLowerCase()
return props.conversationsList.filter(conversation =>
conversation.displayName.toLowerCase().includes(lowerSearchText)
|| conversation.name.toLowerCase().includes(lowerSearchText)
)
} else {
return []
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This component is never used when searchText is '' so isFocused and entire if doesn't do anything here...

src/components/LeftSidebar/SearchConversationsResults/SearchConversationsResults.vue
Comment on lines +219 to +230
const computedStyleCaption = computed(() => {
return {
height: itemSize.value + 'px',
alignItems: props.isCompact ? 'unset' : 'self-end',
}
})

const computedStyleFooter = computed(() => {
return {
marginBlockStart: props.isCompact ? '0' : '18px', // 54px (item height) - 36px (current height)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: let's move it to the template to have all the layout in one place
We can keep computed for the value of marginBlockStart to have it commented here, having only the CSS in the template (less CSS in JS).

src/components/LeftSidebar/SearchConversationsResults/SearchConversationsResults.vue
Comment on lines +185 to +189
const sourcesWithoutResults = computed(() => {
return !hasResultsUsers.value
|| !hasResultsGroups.value
|| (isCirclesEnabled && !hasResultsCircles.value)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

To make it clearer a boolean

Suggested change
const sourcesWithoutResults = computed(() => {
return !hasResultsUsers.value
|| !hasResultsGroups.value
|| (isCirclesEnabled && !hasResultsCircles.value)
})
const hasSourcesWithoutResults = computed(() => {
return !hasResultsUsers.value
|| !hasResultsGroups.value
|| (isCirclesEnabled && !hasResultsCircles.value)
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can only use sourcesWithoutResultsList

src/components/LeftSidebar/SearchConversationsResults/SearchConversationsResults.vue
return isCirclesEnabled && !hasResultsCircles.value
})

const sourcesWithoutResultsList = computed(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: If we rename sourcesWithoutResults, when this doesn't need to have List

Suggested change
const sourcesWithoutResultsList = computed(() => {
const sourcesWithoutResults = computed(() => {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShGKme ShGKme ShGKme left review comments

@Antreesy Antreesy Antreesy left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@DorraJaouad DorraJaouad

Labels
enhancement feature: conversations 👥 feature: frontend 🖌️ "Web UI" client technical debt
Projects
💬 Talk team
Status: 🏗️ In progress
Milestone
🪺 Next Major (32)
Development

Successfully merging this pull request may close these issues.

🔍 Fix infinite scrolling issue in left sidebar when searching with 1k conversations
3 participants
@DorraJaouad @ShGKme @Antreesy