pleroma/pleroma-fe
Watch
2
2
Fork
You've already forked pleroma-fe
 10
Code Issues 351 Pull requests 26 Projects Releases 5 Packages 1 Wiki Activity

Add user search at #2119

Closed
wyatt777 wants to merge 8 commits from gitlab-mr-iid-852 into develop
pull from: gitlab-mr-iid-852
merge into: pleroma:develop
Conversation 25 Commits 8 Files changed 3 +22 -5
wyatt777 commented 2019-06-24 02:42:18 +00:00
Member
Copy link

For issue #562

For issue #562
hj commented 2019-06-24 07:53:30 +00:00
Member
Copy link

probably should throttle/debounce the dispatch so that it doesn't get sent too frequently

probably should throttle/debounce the dispatch so that it doesn't get sent too frequently
wyatt777 commented 2019-06-25 04:48:19 +00:00
Author
Member
Copy link

I added debounce for this.

It might be good to add debounce or throttle as an option in the api. services/new_api/utils.js might be one place.

I added debounce for this. It might be good to add debounce or throttle as an option in the api. `services/new_api/utils.js` might be one place.
hj commented 2019-07-10 16:15:27 +00:00
Member
Copy link

why

why
wyatt777 commented 2019-07-11 00:31:30 +00:00
Author
Member
Copy link

?? More details ??

Following the design pattern that already exists in the same file.

suggestor.js has no reference to this.$store. Need it for userSearch dispatch.

statusPoster in the same file, has the same design pattern. Store is passed to dispatch addNewStatuses.

?? More details ?? Following the design pattern that already exists in the same file. `suggestor.js` has no reference to `this.$store`. Need it for userSearch dispatch. `statusPoster` in the same file, has the same design pattern. Store is passed to dispatch addNewStatuses.
hj commented 2019-07-11 10:21:09 +00:00
Member
Copy link

you're passing entire vuex store into suggestor, de-facto making it dependent on it. This is just bad architectural design. Better solution would be to make users property take usersList and usersFindFunction

you're passing **entire** vuex store into suggestor, de-facto making it dependent on it. This is just bad architectural design. Better solution would be to make `users` property take `usersList` and `usersFindFunction`
hj commented 2019-07-11 10:22:04 +00:00
Member
Copy link

also this only affects post form, should also affect bio input

also this only affects post form, should also affect bio input
wyatt777 commented 2019-07-12 02:05:33 +00:00
Author
Member
Copy link

Image_2019-07-12_at_11.04.18_AM

In the same file the exact same thing is already occurring elsewhere. That should be changed too?

![Image_2019-07-12_at_11.04.18_AM](/attachments/254471bb-22b9-4365-9a6e-712b350c431e) In the same file the exact same thing is already occurring elsewhere. That should be changed too?
Image_2019-07-12_at_11.04.18_AM.png
150 KiB
wyatt777 commented 2019-07-12 02:25:37 +00:00
Author
Member
Copy link

Here is a comparative flow:

statusPoster passes the entire vuex store. It (dispatches) -> addNewStatuses which depends on the store.

emojiUserSuggestor passes the entire vuex store.. It (dispatches) -> searchUsers which depends on the store.

modules/users.js already defines searchUsers which does exactly what we want.

Here is a comparative flow: statusPoster passes the **entire** vuex store. It (dispatches) -> addNewStatuses which depends on the store. emojiUserSuggestor passes the **entire** vuex store.. It (dispatches) -> searchUsers which depends on the store. `modules/users.js` already defines `searchUsers` which does exactly what we want.
wyatt777 commented 2019-07-12 03:29:57 +00:00
Author
Member
Copy link

added!

added!
hj commented 2019-07-12 10:59:01 +00:00
Member
Copy link

oh boy, time for git blame!

oh boy, time for git blame!
hj commented 2019-07-12 11:02:49 +00:00
Member
Copy link

yeah, it's the oldest code, and it's certainly not the way to go IMO. For post_status_form it should've been something like

onMakePost: (data) => store.dispatch('addNewStatuses', {
  statuses: [data],
  timeline: 'friends',
  showImmediately: true,
  noIdUpdate: true // To prevent missing notices on next pull.
})
yeah, it's the oldest code, and it's certainly not the way to go IMO. For post_status_form it should've been something like ```js onMakePost: (data) => store.dispatch('addNewStatuses', { statuses: [data], timeline: 'friends', showImmediately: true, noIdUpdate: true // To prevent missing notices on next pull. }) ```
hj commented 2019-07-12 11:06:54 +00:00
Member
Copy link

for emoji suggestor is should be something like

{
  emoji: [
    ...this.$store.state.instance.emoji,
    ...this.$store.state.instance.customEmoji
  ],
  users: {
    list: this.$store.state.users.users,
    updateList: (input) => data.store.dispatch('searchUsers', input)
  }
}

although ideally i'd make emoji suggestor asynchronous and make it use results from request directly, but it could be next step.

for emoji suggestor is should be something like ```js { emoji: [ ...this.$store.state.instance.emoji, ...this.$store.state.instance.customEmoji ], users: { list: this.$store.state.users.users, updateList: (input) => data.store.dispatch('searchUsers', input) } } ``` although ideally i'd make emoji suggestor asynchronous and make it use results from request directly, but it could be next step.
wyatt777 commented 2019-07-12 14:04:32 +00:00
Author
Member
Copy link

Cool! Yes git blame. Want to keep it in the order that is already established, possibly multiple places in the code that are using the same logic and changing that deserves a discussion.

Cool! Yes git blame. Want to keep it in the order that is already established, possibly multiple places in the code that are using the same logic and changing that deserves a discussion.
tae commented 2019-07-14 20:52:29 +00:00
Member
Copy link

I can see the concerns here.
hj wants to nicely abstract that logic (data.store.dispatch('searchUsers', input)) away from suggestor.js and make the updating logic available and customizable on-demand to any component that needs it that makes sense to me. 👍

@wyatt777 It'd be great if you can follow the pattern so suggestor.js doesn't need to know the whole vuex store logic. Otherwise, we can just pass this.$store only and destructure everything in suggestor.js. 😉

As you mentioned, we have duplicated logics of emojiUserSuggestor and emojiSuggestor in two places. We definitely want abstraction that allows us to define emojiUserSuggestor and emojiSuggestor in a single place and share it across many components, but we can try it in another MR later.

I can see the concerns here. hj wants to nicely abstract that logic (`data.store.dispatch('searchUsers', input)`) away from `suggestor.js` and make the updating logic available and customizable on-demand to any component that needs it that makes sense to me. :thumbsup: @wyatt777 It'd be great if you can follow the pattern so `suggestor.js` doesn't need to know the whole vuex store logic. Otherwise, we can just pass `this.$store` only and destructure everything in `suggestor.js`. :wink: As you mentioned, we have duplicated logics of `emojiUserSuggestor` and `emojiSuggestor` in two places. We definitely want abstraction that allows us to define `emojiUserSuggestor` and `emojiSuggestor` in a single place and share it across many components, but we can try it in another MR later.
wyatt777 commented 2019-07-15 01:38:19 +00:00
Author
Member
Copy link

Hi @tae ! Yes I agree with the changes and will do a push with them for my function.

For future proofing:
I think doing it right once across the whole code base is better than choosing one place randomly. If there are multiple objects passing the $store there should be an issue that looks into that specifically and makes the code have consistent treatment of them.

Hi @tae ! Yes I agree with the changes and will do a push with them for my function. For future proofing: I think doing it right once across the whole code base is better than choosing one place randomly. If there are multiple objects passing the `$store` there should be an issue that looks into that specifically and makes the code have consistent treatment of them.
tae commented 2019-07-15 03:50:23 +00:00
Member
Copy link

lgtm

lgtm
hj commented 2019-07-15 05:58:24 +00:00
Member
Copy link
  1. update the comment above so that it also explains it
  2. naming - it's not obvious from the name that updateList updates list of users. should either be a property of data.users or something like updateUsersList
  3. make it optional so that it doesn't explode when it's not defined?
1. update the comment above so that it also explains it 2. naming - it's not obvious from the name that `updateList` updates list of users. should either be a property of `data.users` or something like `updateUsersList` 3. make it optional so that it doesn't explode when it's not defined?
hj commented 2019-07-15 06:38:06 +00:00
Member
Copy link

other than that - lgtm

other than that - lgtm
wyatt777 commented 2019-07-15 23:17:27 +00:00
Author
Member
Copy link

1,2 & 3. Pushed.

1,2 & 3. Pushed.
hj commented 2019-07-16 06:18:22 +00:00
Member
Copy link

data.users.indexOf(input) === -1

will not work. input is string, data.users is array of objects, it will always be true and always iterate over entire array.

`data.users.indexOf(input) === -1` will not work. `input` is string, `data.users` is array of objects, it will always be true and always iterate over entire array.
wyatt777 commented 2019-07-16 06:22:06 +00:00
Author
Member
Copy link

@hj Thank you. Will fix it and re-push.

@hj Thank you. Will fix it and re-push.
wyatt777 commented 2019-07-16 06:42:51 +00:00
Author
Member
Copy link

@hj I pushed again with the changes. Not sure how much this is needed. It only blocks if there is an exact match within data.users. Maybe there should be an array with all inputs that were already searched and block them all?

@hj I pushed again with the changes. Not sure how much this is needed. It only blocks if there is an exact match within data.users. Maybe there should be an array with all inputs that were already searched and block them all?
hj commented 2019-07-16 07:42:19 +00:00
Member
Copy link

maybe it should be moved into suggestUsers which does all the necessary filtering already?

maybe it should be moved into `suggestUsers` which does all the necessary filtering already?
wyatt777 commented 2019-07-17 03:22:34 +00:00
Author
Member
Copy link

Thanks. It will only search if there are no matches from suggestUsers now.

Thanks. It will only search if there are no matches from `suggestUsers` now.
shpuld commented 2019-07-18 03:39:52 +00:00
Member
Copy link

works for me

works for me

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
BE Request

   
Backlog

   
Better behavior

   
Blocked by deps

   
Bug

   
Code Review

   
Discussion

   
Doing

   
Feature Request / Enhancement

   
In the shining bright future maybe

   
It's complicated

   
MS Edge

   
Mememoon (or other niche browsers)

   
Missing API

   
Missing feature

   
Missing l10n/i18n

   
NL1
Instance Admin Dashboard

  
NL2
Improved Mobile Experience

  
NL4
Improved instance customization and QoL options

  
Need to verify on develop

   
RELEASE BLOCKER

   
Reassign or Close

   
Refactor

   
Regression

   
Safari

   
To Do

   
User story

   
Waiting on godot

   
accessibility
https://git.pleroma.social/pleroma/pleroma-fe/-/snippets/7746

  
better documentation

   
chore

   
confirmed

   
easy ticket

   
incident
Denotes a disruption to IT services and the associated issues require immediate attention

  
l10n update

   
mastoapi

   
mobile

   
needs design

   
needs-info

   
needs-review

   
stupid
No labels
BE Request
Backlog
Better behavior
Blocked by deps
Bug
Code Review
Discussion
Doing
Feature Request / Enhancement
In the shining bright future maybe
It's complicated
MS Edge
Mememoon (or other niche browsers)
Missing API
Missing feature
Missing l10n/i18n
NL1
NL2
NL4
Need to verify on develop
RELEASE BLOCKER
Reassign or Close
Refactor
Regression
Safari
To Do
User story
Waiting on godot
accessibility
better documentation
chore
confirmed
easy ticket
incident
l10n update
mastoapi
mobile
needs design
needs-info
needs-review
stupid
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
pleroma/pleroma-fe!2119
No description provided.