Add autocomplete to add members to a group by name by mgorkove · Pull Request #40 · kernel-community/services

mgorkove · 2022-06-15T20:32:24Z

Screen.Recording.2022-06-14.at.5.44.46.PM.mov

simonkernel

Thanks for working on this!

Just a few smaller nits, the linter had some feedback, and then one main comment is -

The query service can throw for the cases where the user has either no profile or has not consented to sharing their data.

This has the following implications -

need to try/catch
need to still accommodate adding users by member ids directly

Let me know if the comments aren't sufficient and you have more questions!

simonkernel · 2022-06-16T18:51:34Z

packages/common/src/components/AutocompleteInput.jsx

+import { Combobox } from '@headlessui/react'
+import { XIcon } from '@heroicons/react/outline'
+
+export default function AutocompleteInput(props) {


nit: just destructure in args -

export default function AutocompleteInput({ items, selectedItems, setSelectedItems }) {

simonkernel · 2022-06-16T18:54:28Z

packages/common/src/components/AutocompleteInput.jsx

+      {selectedItems.length > 0 && (
+        <div className='flex mt-1'>
+          {selectedItems.map((item) => (
+            <div key={item.id} className='flex p-1 border-solid border-2 border-gray-300 rounded-md mr-2'>{item.name} <XIcon className='w-4 ml-3' onClick={() => removeSelectedItem(item.id)}/></div>


nit: break up line so it's more readable

simonkernel · 2022-06-16T18:55:17Z

packages/common/src/components/AutocompleteInput.jsx

+      />
+      {selectedItems.length > 0 && (
+        <div className='flex mt-1'>
+          {selectedItems.map((item) => (


nit: destructure item

...map(({ id, name }=> (

simonkernel · 2022-06-16T19:10:57Z

packages/groups/src/components/Form.jsx

+const getMemberIds = (groupMembers) => groupMembers.map(groupMember => groupMember.id)
+const transformProfiles = (profiles) => {
+  const out = []
+  for (const [, v] of Object.entries(profiles)) {


nit: no need to create a temporary array and can just map directly

... => Object.values(profiles).map(({ data: { memberId, name }}) => ({ id: memberId, name }))

simonkernel · 2022-06-16T19:21:05Z

packages/groups/src/components/Form.jsx

      const members = await entityFactory({ resource: 'member' })
      const member = await members.get(user.iss)
      const groups = await entityFactory({ resource: 'group' })
+      const { profiles } = await queryService.recommend()


This can throw an exception when the user making this call either has no profile or did not consent to sharing their data [0]

We should wrap this call in a try/catch to not break the app.

[0] https://github.com/kernel-community/services/blob/main/packages/query/src/services/query.js#L58

simonkernel · 2022-06-16T19:26:27Z

packages/common/src/components/AutocompleteInput.jsx

+  const filteredItems =
+    query === ''
+      ? items
+      : items.filter((item) => {


nit: shorten to

items.filter(({ name }) => name.toLowerCase().includes(query.toLowerCase()))

simonkernel · 2022-06-16T19:32:09Z

packages/common/src/components/AutocompleteInput.jsx

+
+  return (
+    <Combobox value={selectedItems} onChange={setSelectedItems} multiple>
+      <Combobox.Input


We need to allow for custom values so we can still add members by id only [0]

[0] https://headlessui.dev/react/combobox#allowing-custom-values

simonkernel · 2022-06-16T19:32:56Z

packages/groups/src/components/Form.jsx

-  const memberIds = textToArray(memberIdsText)
-  if (!name.length || !memberIdsText.length) {
+  const { groups, groupMembers, name, taskService } = state
+  const memberIds = getMemberIds(groupMembers)


Needs to support the case when a member is added by id instead of name.

simonkernel · 2022-06-16T19:33:07Z

packages/groups/src/components/Form.jsx

+  const { group, groups, groupMembers, name, taskService } = state
  const groupId = group.id
-  const memberIds = textToArray(memberIdsText)
+  const memberIds = getMemberIds(groupMembers)


simonkernel · 2022-06-16T19:35:23Z

package.json

-  "license": "MIT"
+  "license": "MIT",
+  "dependencies": {
+    "@heroicons/react": "^1.0.6"


how useful do we think this library is in the future?

we should debate any additional dependency and ensure there is significant value as a whole outweighing the risks of an added dep.

alternatively, if we think we just need this one SVG, we can always just copy it over directly into the repo and later add the dep if we end up using a lot of different icons.

mgorkove · 2022-06-19T05:46:35Z

Hi @mgorkove,

Thanks for working on this!

Just a few smaller nits, the linter had some feedback, and then one main comment is -

The query service can throw for the cases where the user has either no profile or has not consented to sharing their data.

This has the following implications -

need to try/catch

need to still accommodate adding users by member ids directly

Let me know if the comments aren't sufficient and you have more questions!

@simonkernel do we really need to accommodate still adding by member ids directly? Wasn't the whole point of the autocomplete to be able to get rid of that?

simonkernel · 2022-06-20T19:05:23Z

Hi @mgorkove,

Yes we do need adding members by id. Also, we actually also need to display the member id so members with the same name can be distinguished. We can just add it in parentheses ex Rory (1234567).

Thanks,
Simon

mgorkove requested a review from rorysaur June 15, 2022 20:34

simonkernel suggested changes Jun 16, 2022

View reviewed changes

added query service endpoints to .env

7dffe8c

mgorkove force-pushed the autocomplete branch 2 times, most recently from 849eab9 to df6c3d4 Compare June 19, 2022 06:58

added autocomplete component

dce4f2a

mgorkove force-pushed the autocomplete branch from df6c3d4 to 4f572cd Compare June 20, 2022 01:56

moved readable to common

de3c152

mgorkove force-pushed the autocomplete branch 2 times, most recently from e19dc1c to 1e6b10e Compare June 20, 2022 02:16

replaced memberIds input with autocomplete

c1452cc

mgorkove force-pushed the autocomplete branch from 1e6b10e to c1452cc Compare June 20, 2022 02:21

Comments

Conversation

mgorkove commented Jun 15, 2022

Uh oh!

simonkernel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgorkove commented Jun 19, 2022

Uh oh!

simonkernel commented Jun 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants