Skip to content

Conversation

@mejo-
Copy link
Member

@mejo- mejo- commented Jan 12, 2023

📝 Summary

🖼️ Screenshots

Enter name Select Circle Select Members
grafik grafik grafik

Screencast

recording.webm

🚧 TODO

  • Fix cropped bottom in member suggestions
  • Fix member types in onCreate() before giving members to ADD_MEMBERS_TO_CIRCLE
  • Cypress tests

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@mejo- mejo- added enhancement New feature or request 2. developing labels Jan 12, 2023
@mejo- mejo- marked this pull request as draft January 12, 2023 12:42
@mejo- mejo- force-pushed the enh/create_collective branch 2 times, most recently from 819ba63 to c180284 Compare January 12, 2023 12:54
@cypress
Copy link

cypress bot commented Jan 12, 2023

1 flaky tests on run #368 ↗︎

0 61 0 0 Flakiness 1

Details:

Replace the 'new collective' form with a modal
Project: Collectives Commit: eca453b3b0
Status: Passed Duration: 04:22 💡
Started: Feb 21, 2023 2:09 PM Ended: Feb 21, 2023 2:13 PM
Flakiness  cypress/e2e/collective.spec.js • 1 flaky test

View Output Video

Test
Collective > after creation > has all the ui elements Screenshot

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@mejo- mejo- force-pushed the enh/create_collective branch 2 times, most recently from 68f5eb9 to f554c22 Compare January 12, 2023 14:33
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.

Looks very nice, very much like in Talk :) cc @marcoambrosini. 2 pieces of feedback:

  • Also like in Talk, in the first step we could already list some permissions, e.g. the editing and sharing permissions, as well as the default page mode? Then it’s not so empty, and the permissions are set before anything is shared, very secure.
  • The "Collective with that name already exists" could already be shown in the name input step, no? (Like how web services often check in the background if the account name is still available.)

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Super nice! Flow looks really great :) Agreed with @jancborchardt plus some points from my side:

  • The icon for selecting a circle seems unclear. Maybe it could a button with a label "Choose circle" or something like that?
  • "Select members" --> "Add people"? cc @jancborchardt on this one
  • It also seems like it's confusing to create a personal collective. We could possibly add a secondary button on the left of "Select members" which says "Create" or something. also cc @jancborchardt
  • Hover feedback for the emoji seems to be a few pixels too wide, it should be a circle (equal width and height)
  • Each item in the list of users doesn't need to have a background, background shown only on hover and select would work better :)
  • The checkmark could have more margin all around. Even better: Align all checkmarks to the right, similar to creating a new group conversation in Talk
    image
  • Also a question: what is the symbol in the search field on the right?
    image

@julien-nc
Copy link
Member

julien-nc commented Jan 12, 2023

Wow that looks very nice! 🤩

  • The bottom right button looks the same when you can't click it (for example "Select members" is disabled when no collective name was entered yet) and when something is loading (for example "Create" is disabled when attempting to create the collective). There could be a loading icon in the button's icon template while creating.
  • There could be an empty content while choosing the collective name, there seems to be one only while choosing the members. The empty content icon could be the app icon and the title could be something like "Enter the new collective name". The icon alone without title would be fine too.

@mejo- mejo- mentioned this pull request Feb 13, 2023
4 tasks
@mejo-
Copy link
Member Author

mejo- commented Feb 14, 2023

  • Also like in Talk, in the first step we could already list some permissions, e.g. the editing and sharing permissions, as well as the default page mode? Then it’s not so empty, and the permissions are set before anything is shared, very secure.

In the long run I thought that we could provide templates for the new collective here. That's why I kept it empty for now. Sure, we could add settings there, but I'd like to do this in a follow-up if at all, as it requires quite a bit of extra work.

* [ ]  The "Collective with that name already exists" could already be shown in the name input step, no? (Like how web services often check in the background if the account name is still available.)

Yeah, that would be a nice user experience. On the other hand, that would require an API call to check if a circle with a specifici name exists (circle names are global on the instance) and that would mean to allow enumeration. At the moment, when creating a collective, the backend tries to create the circle and errors out if it already exists. That's way harder to brute-force than an API that allows to check if a name is taken.

@mejo-
Copy link
Member Author

mejo- commented Feb 14, 2023

Also a question: what is the symbol in the search field on the right?

Hehe, that's my password manager 😉

@mejo- mejo- force-pushed the enh/create_collective branch 3 times, most recently from 7f4e65a to d33e85a Compare February 15, 2023 13:46
@mejo-
Copy link
Member Author

mejo- commented Feb 15, 2023

@nimishavijay @jancborchardt @julien-nc I tried to address all your feedback, thanks a lot. Some ideas I didn't implement, but I commented on them instead, see above. Screenshots and screencast above are updated as well.

@mejo- mejo- force-pushed the enh/create_collective branch from d33e85a to c3fd35e Compare February 15, 2023 14:40
@mejo- mejo- marked this pull request as ready for review February 15, 2023 14:40
@susnux
Copy link
Contributor

susnux commented Feb 15, 2023

Looks really nice!
I will look into this later, but from a quick look on the code: Is there a special reason for not using the NcSelect component for the member picker? I think it would simplify the code a lot.

@mejo-
Copy link
Member Author

mejo- commented Feb 15, 2023

Looks really nice! I will look into this later, but from a quick look on the code: Is there a special reason for not using the NcSelect component for the member picker? I think it would simplify the code a lot.

Thanks for looking into it @susnux! The main reason is that the member picker dialog should look similar to how it looks in Talk when creating a new conversation and in Contacts when picking members for a circle. I agree that we should streamline this into a nextcloud-vue component at some point.

@mejo- mejo- force-pushed the enh/create_collective branch from d5498c6 to 3f28b8a Compare February 15, 2023 15:33
@mejo- mejo- force-pushed the enh/create_collective branch 2 times, most recently from 95cad3e to c6bcbd6 Compare February 20, 2023 11:31
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Just had a look at the code so far.
Looks good to me. 🎉
Just some minor comments.

Copy link
Member

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Looks great! Only point I noticed is the button for choosing a circle is a bit confusing without a label, but we can merge this now and open a follow up issue about it :)

* Fix emoji button width to 44px
* "Select member" => "Add people"
* No default background for member search results
* Align checkmarks for selected member search results to right
* Fix invisible circle picker dropdown menu
* Add min-height to selected members area to prevent content jumping
* Clear search results before starting a new search

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- force-pushed the enh/create_collective branch from c6bcbd6 to 3eba24e Compare February 21, 2023 13:35
* Remove dead code that dealt with duplicate diplayNames in search
  results. We don't use displayName at all.
* Move code to filter and sort results into its own helper function.
* Further minor improvements.

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- force-pushed the enh/create_collective branch from 3eba24e to eca453b Compare February 21, 2023 13:46
@mejo- mejo- merged commit 080e3f8 into main Feb 21, 2023
@delete-merged-branch delete-merged-branch bot deleted the enh/create_collective branch February 21, 2023 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developing enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improved dialog to create new collective

8 participants