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

Sort groups alphabetically instead of by amount of contacts #1461

Closed
wants to merge 2 commits into from

Conversation

ImUrX
Copy link
Member

@ImUrX ImUrX commented Feb 2, 2020

Just redid the branch of #1451 because accidentaly deleted package-lock.json
This PR should fix #522
I still would like to test this on windows, I dont know how to do so, I just been reading the code a lot to see if I'm doing something wrong :p

@ImUrX ImUrX requested a review from skjnldsv February 2, 2020 15:33
@codecov
Copy link

codecov bot commented Feb 2, 2020

Codecov Report

Merging #1461 into master will increase coverage by 4.84%.
The diff coverage is 40.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1461      +/-   ##
============================================
+ Coverage     62.31%   67.16%   +4.84%     
- Complexity        0       15      +15     
============================================
  Files             4        5       +1     
  Lines            69       67       -2     
============================================
+ Hits             43       45       +2     
+ Misses           26       22       -4     
Impacted Files Coverage Δ Complexity Δ
appinfo/app.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
lib/AppInfo/Application.php 0.00% <0.00%> (ø) 2.00 <2.00> (?)
lib/Controller/PageController.php 100.00% <100.00%> (+20.00%) 2.00 <1.00> (+2.00)
lib/ContactsMenu/Providers/DetailsProvider.php 96.87% <0.00%> (+10.76%) 11.00% <0.00%> (+11.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df46053...c7079d1. Read the comment docs.

@skjnldsv
Copy link
Member

skjnldsv commented Feb 2, 2020

Great! :)
To answer your other question, it should be default as it is (by count then fallback by name)

Thanks again, super great having awesome contributors like you! 🤗

@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Feb 2, 2020
src/store/groups.js Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments :)

@ImUrX ImUrX requested a review from skjnldsv February 3, 2020 08:53
src/store/groups.js Outdated Show resolved Hide resolved
css/SettingsSection.scss Outdated Show resolved Hide resolved
src/store/groups.js Outdated Show resolved Hide resolved
src/store/groups.js Outdated Show resolved Hide resolved
@ImUrX ImUrX requested a review from skjnldsv February 3, 2020 10:32
src/store/groups.js Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 3, 2020
src/store/groups.js Outdated Show resolved Hide resolved
@ImUrX
Copy link
Member Author

ImUrX commented Feb 3, 2020

yay, lets hope the PR does what it should do

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

  • No default selected and changing doesn't work (no error in logs)
    image
  • Hard to differentiate the two dropdown for contacts and groups sorting. Shall we split with the import dropdown above and change the labels? `Sort contacts by xxx', 'Sort groups by xxx' ?

src/store/groups.js Outdated Show resolved Hide resolved
src/store/groups.js Outdated Show resolved Hide resolved
src/store/groups.js Outdated Show resolved Hide resolved
@jancborchardt
Copy link
Member

Getting a different console error now, navigation not showing up either ;)

[Vue warn]: Error in render: "TypeError: matchB is null"

found in

---> <Contacts> at src/views/Contacts.vue
       <ContactsRoot> at src/ContactsRoot.vue
         <Root> vue.runtime.esm.js:619
TypeError: "matchB is null"
    menu Contacts.vue:277
    groupsMenu Contacts.vue:273
    VueJS 3
    menu Contacts.vue:291
    VueJS 4
    render Contacts.vue:31
    VueJS 7
vue.runtime.esm.js:1888

@ImUrX
Copy link
Member Author

ImUrX commented May 23, 2020

i tried to detect the null with a nor but i should have used a nand

@jancborchardt
Copy link
Member

Now it works but one group with a "V" is sorted up top? (Has 1 person if that is relevant.)
sort

@ImUrX
Copy link
Member Author

ImUrX commented May 24, 2020

mmm this needs debugging but idk how to compile this. The weird part is that the only thing that should be pulled up first is group.name === t('contactsinteraction', 'Recently contacted')

@jancborchardt
Copy link
Member

Hmm, actually "recently contacted" doesn't show in the list at all. Did I miss anything except checking out the branch and npm ci && npm run dev?

@ImUrX
Copy link
Member Author

ImUrX commented May 24, 2020

No idea honestly, been trying to figure it out myself

@jancborchardt
Copy link
Member

Any idea @nextcloud/contacts?

@jancborchardt
Copy link
Member

Now it works but one group with a "V" is sorted up top?

For reference: If I delete that group, the next one from the end of the alphabet is shown there. "Recently contacted" is not there, and I’m guessing the bug has to do with that.

@ImUrX
Copy link
Member Author

ImUrX commented May 24, 2020

if there is no group with the name Recently Contacted then findIndex should be returning -1, slice shouldnt do anything with -1 tho.

@jancborchardt
Copy link
Member

@ImUrX very nice, works well now! :) Could you squash your commits into one, and also sign the commit? (You can sign the commit retroactively via git commit --amend -s.)

@jancborchardt
Copy link
Member

And @call-me-matt could you also look over the code? :)

@ImUrX we have a chat channel for Contacts app contributors, would you like to be invited there? :)

@ImUrX
Copy link
Member Author

ImUrX commented May 25, 2020

yeah sure

Signed-off-by: ImUrX <urielfontan2002@gmail.com>

Fixes #1598 and #1599

Signed-off-by: ImUrX <urielfontan2002@gmail.com>

object is not a string

Signed-off-by: ImUrX <urielfontan2002@gmail.com>

use an and not an or

Signed-off-by: ImUrX <urielfontan2002@gmail.com>

check if findIndex gives -1

Signed-off-by: ImUrX <urielfontan2002@gmail.com>
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Works very nicely design-wise, but needs a code review @nextcloud/contacts :)

src/views/Contacts.vue Outdated Show resolved Hide resolved
@ImUrX ImUrX requested a review from skjnldsv June 7, 2020 07:30
skjnldsv added a commit that referenced this pull request Jul 4, 2020
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member

skjnldsv commented Jul 4, 2020

Hey!
This will be merged part of the new group enhancement pull request because I changed the way roups menu is generated! I will use your logic because I find it clean :)
So don't worry if this is just getting closed and not merged, this will be part of #1687

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 4, 2020
skjnldsv added a commit that referenced this pull request Jul 14, 2020
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv closed this in ec9b6ed Aug 21, 2020
@skjnldsv skjnldsv modified the milestones: next minor, next Sep 6, 2020
@skjnldsv skjnldsv modified the milestones: next, 3.4.0 Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Recently contacted" should be sorted further up top as it is a special group Groups sorting in browser
3 participants