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

Move contactsmenu to vue #22274

Closed
wants to merge 1 commit into from
Closed

Move contactsmenu to vue #22274

wants to merge 1 commit into from

Conversation

dk1a
Copy link

@dk1a dk1a commented Aug 17, 2020 •
edited
Loading

Fix #12905
Fix #17483
Signed-off-by: Kirill Dmitriev dk1a@protonmail.com

@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch from 4f10fe4 to 2d5fca4 Compare August 17, 2020 20:01
@skjnldsv skjnldsv added this to the Nextcloud 21 milestone Aug 17, 2020
skjnldsv
skjnldsv reviewed Aug 17, 2020
View reviewed changes
core/src/init.js Outdated Show resolved Hide resolved
@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch from 2d5fca4 to a7ce86b Compare August 19, 2020 07:26
@dk1a
Copy link
Author

dk1a commented Aug 19, 2020

I moved the styles to vue components, but also left them in styles.scss, since it seems required by the jquery version of contactsmenu, not sure what to do with that one

@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch from a7ce86b to e9d6ac8 Compare August 19, 2020 07:51
@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch from e9d6ac8 to 0519568 Compare August 20, 2020 06:16
skjnldsv
skjnldsv requested changes Aug 20, 2020
View reviewed changes
core/src/views/ContactsMenu.vue Outdated Show resolved Hide resolved
core/src/views/ContactsMenu.vue Outdated Show resolved Hide resolved
core/src/views/ContactsMenu.vue

onInput: debounce(function(query) {
// load contacts on input
this.loadContacts()
Copy link
Member

Choose a reason for hiding this comment

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

We need to comply to the min search length

server/lib/private/Contacts/ContactsMenu/Manager.php

Line 66 in 7c56283

$minSearchStringLength = $this->config->getSystemValueInt('sharing.minSearchStringLength', 0);

Copy link
Author

Choose a reason for hiding this comment

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

changed this in a previous commit; though is it intended that with minSearchStringLength>0 the contacts menu will be empty initially? That's how the backend did it, so I copied the behavior to frontend

core/src/views/ContactsMenu.vue Outdated Show resolved Hide resolved
@jancborchardt
Copy link
Member

jancborchardt commented Aug 20, 2020 •
edited
Loading

Very very cool @dk1a! :) Awesome that you are looking at the Contacts menu.

Opened a new issue with possible further enhancements which we had in mind for some time – the Vue rewrite could be a good base for this! #22329

@skjnldsv
Copy link
Member

If you use the contacts vcf url and add ?photo at the end, it will try to fetch the avatar.
Could be a nice combo with the Avatar component (that will fallback to a placeholder if the avatar image load fails)
ref #17483

@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch from 0519568 to 4f68cb1 Compare August 20, 2020 20:47
@dk1a
Copy link
Author

dk1a commented Aug 20, 2020

If you use the contacts vcf url and add ?photo at the end, it will try to fetch the avatar.
Could be a nice combo with the Avatar component (that will fallback to a placeholder if the avatar image load fails)
ref #17483

Contacts already returned an avatar image, it just didn't work on the backend - fixed it

@dk1a dk1a mentioned this pull request Aug 20, 2020
Closed
2 tasks
skjnldsv
skjnldsv reviewed Aug 21, 2020
View reviewed changes
apps/accessibility/css/dark.scss Outdated
@@ -19,7 +19,7 @@ $color-border-dark: lighten($color-main-background, 14%);

#app-navigation > ul > li > a:first-child,
#app-navigation > ul > li > ul > li > a:first-child,
#contactsmenu-menu a,
.contacts-menu__contacts-list a,
Copy link
Member

Choose a reason for hiding this comment

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

You can do it directly in the ContactsMenu.
Use body.theme--dark to narrow the selector :)

@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch 2 times, most recently from b6185d2 to 00d779a Compare August 21, 2020 21:17
@skjnldsv skjnldsv linked an issue Aug 25, 2020 that may be closed by this pull request
Vue contacts menu #15586
Closed
@skjnldsv
Copy link
Member

Any news @dk1a ? :)

@dk1a
Move contactsmenu to vue
0a1a987 
Signed-off-by: Kirill Dmitriev <dk1a@protonmail.com>
@dk1a dk1a force-pushed the move_contactsmenu_to_vue branch from 00d779a to 0a1a987 Compare August 25, 2020 18:17
@dk1a
Copy link
Author

dk1a commented Aug 25, 2020

I'd added min search length, moved dark style;
was also going to look at what other apps do with contactsmenu sometime this week.
Test failures seem unrelated, are they a problem?

@skjnldsv
Copy link
Member

Jsunit is legit.
Node / build too :)

@jancborchardt
Copy link
Member

added min search length

Do we really want min search length? UX wise it’s always a bit of a hassle, and the current Contacts menu doesn’t have any minimum either.

@skjnldsv
Copy link
Member

Do we really want min search length? UX wise it’s always a bit of a hassle, and the current Contacts menu doesn’t have any minimum either.

we have it everywhere. Unified search, sharing, contactsmenu... etc

@skjnldsv
Copy link
Member

skjnldsv commented Nov 7, 2020

Hey! Any news on this? :)
Now that we are into 21, could you rebase?
If you want to join, we have a community channel where we chat 😉
Are you interested?

@skjnldsv skjnldsv mentioned this pull request Nov 13, 2020
Merged
This was referenced Dec 14, 2020
Merged
Merged
@ChristophWurst
Copy link
Member

Now that we are into 21, could you rebase?

We're in beta now. I'd say let's get this in in a few weeks when 21 was branched off. @dk1a could you please rebase and rebuild your branch to resolve the conflicts? Thanks ✌️

@MorrisJobke MorrisJobke mentioned this pull request May 20, 2021
Merged
@MorrisJobke
Copy link
Member

Let's close this for now. We can reopen it at any time again, but this one here just didn't got any update recently.

@szaimen
Copy link
Contributor

szaimen commented May 20, 2021

@MorrisJobke May I suggest to just get rid of the contacts menu now that we have unified search?

@MorrisJobke
Copy link
Member

@MorrisJobke May I suggest to just get rid of the contacts menu now that we have unified search?

cc @skjnldsv @ChristophWurst

@skjnldsv skjnldsv mentioned this pull request Nov 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjnldsv skjnldsv skjnldsv requested changes

Assignees
No one assigned
Labels
2. developing Work in progress enhancement feature: contacts menu technical debt
Projects
None yet
Milestone
Nextcloud 22
Development

Successfully merging this pull request may close these issues.

contact search: Avatar images doesn't show up Vue contacts menu Move contactsmenu to vue
6 participants
@dk1a @jancborchardt @skjnldsv @ChristophWurst @MorrisJobke @szaimen