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

Flatten left sidebar nested lists #9068

Merged
merged 3 commits into from
Apr 5, 2023

Conversation

marcoambrosini
Copy link
Member

No description provided.

@nickvergessen
Copy link
Member

What's the target date with this? It will "break" very soon if anyone touches the involved files

@marcoambrosini marcoambrosini force-pushed the feature/noid/flatten-conversations-list-2 branch 2 times, most recently from d42bc3f to a4a9112 Compare March 28, 2023 16:43
@nickvergessen
Copy link
Member

Conflicting files
src/components/LeftSidebar/LeftSidebar.vue

src/components/LeftSidebar/LeftSidebar.spec.js Outdated Show resolved Hide resolved

expect(conversationsReceivedEvent).not.toHaveBeenCalled()

// move on past the fetchConversation call
await wrapper.vm.$nextTick()
await wrapper.vm.$nextTick()

expect(listEl.props('initialisedConversations')).toBe(true)
expect(listEl.props('conversationsList')).toStrictEqual([
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 be order of conversations checked and tested for this component here and in other places?
Suitable places, in my opinion:

  • initial render (list and search list)
  • after list refetching
  • after lastActivity changing
  • after new conversation creating
  • with favorite conversations (1 and more)

I guess, they come from server in the right order, so we could emulate it with:

const sortedConversationList = conversationsList.slice().sort((a, b) => a.lastActivity - b.lastActivity)

and test like this:

conversationArray.wrappers.forEach((conversation, index) => {
    expect(conversation.props('displayName').toBe(sortedConversationList[index].displayName)
}

Please check if it works properly, i didn't test it
Would be nice to hear @ShGKme opinion on this

src/components/LeftSidebar/LeftSidebar.vue Show resolved Hide resolved
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Add some fixes in commits below

@Antreesy Antreesy force-pushed the feature/noid/flatten-conversations-list-2 branch 3 times, most recently from cc55983 to 0e8a5ff Compare April 1, 2023 12:31
@Antreesy Antreesy requested a review from ShGKme April 1, 2023 12:32
marcoambrosini and others added 3 commits April 5, 2023 16:14
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the feature/noid/flatten-conversations-list-2 branch from 0e8a5ff to 9afe1ab Compare April 5, 2023 14:19
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 fine after the last rebase and flattened. Tested with different searches, mentions, unread notifications, refreshing, on mobile.

There are things to refactor, but we may do it late, as it is urgent and refactoring is not the purpose of the PR.

@Antreesy Antreesy enabled auto-merge April 5, 2023 16:36
@Antreesy
Copy link
Contributor

Antreesy commented Apr 5, 2023

/backport to stable26

⚠️ master branch have some changes from #9054 in ConversationList.vue that needs to be approved

@Antreesy Antreesy merged commit e39bd72 into master Apr 5, 2023
17 checks passed
@Antreesy Antreesy deleted the feature/noid/flatten-conversations-list-2 branch April 5, 2023 16:43
@backportbot-nextcloud
Copy link

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin/stable26

# Create the new backport branch
git checkout -b fix/foo-stable26

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Comment on lines -117 to -119
if (to.name !== 'conversation') {
this.$store.dispatch('updateToken', '')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be added after #9058 approval

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

#9058 is a backport, so it should be fixed anyway.

Copy link
Contributor

@Antreesy Antreesy Apr 5, 2023

Choose a reason for hiding this comment

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

Sadly, but no. It's a copied method from deleted file. Patched this in backport already

ShGKme added a commit that referenced this pull request Apr 5, 2023
Regression of: #9068

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>
@ShGKme
Copy link
Contributor

ShGKme commented Apr 5, 2023

Should we also remove branches:

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.

None yet

4 participants