Introduce modal with thread members. It contains tabs with members and admins lists.
Details
Details
- Reviewers
tomek • benschac atul ashoat - Commits
- rCOMMbc98f9ff50f3: [web] Introduce `MembersModal`
It can be run from thread actions menu after the following diff.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/modals/threads/members/members-modal.css | ||
|---|---|---|
| 6 ↗ | (On Diff #10206) | https://mxstbr.com/thoughts/margin/ -- can we stay away from margin when possible? |
| web/modals/threads/members/members-modal.react.js | ||
| 60 ↗ | (On Diff #10206) | if no children why not: <ThreadMembersList />? |
| 76 ↗ | (On Diff #10206) | why not put setSearch in a useCallback ? |
| web/modals/threads/members/members-modal.react.js | ||
|---|---|---|
| 24 ↗ | (On Diff #10206) | We should define a type that can be All Members | Admins and use it to limit possible tab values |
| 25 ↗ | (On Diff #10206) | Naming it more explicitly to searchText would make it more readable |
| 33 ↗ | (On Diff #10206) | I'm not sure if this is fast enough. Every time a user types a letter, we need to iterate through the whole list of users, which might be slow. The problem is that, when we have a result for some prefix, we could iterate through just the remaining users, and not all of them. We have some searching utils, e.g. SearchIndex - maybe they can be used? Otherwise, maybe there's a library which can be used? We can also use our own solution - the data structure we need is known as trie. http://lyndseyb.co.uk/posts/trie-prefix-tree If we could test this in a thread with about 100 users on a slow phone and no performance issues will be visible, then we can consider keeping the current solution. But if there are performance issues, the solution might be time consuming, so we should prioritize it somehow @ashoat |
| 47–51 ↗ | (On Diff #10206) | |
| 60 ↗ | (On Diff #10206) | |
| 76 ↗ | (On Diff #10206) | It can be just onChangeText={setSearch} |
| web/modals/threads/members/members-modal.css | ||
|---|---|---|
| 6 ↗ | (On Diff #10206) | I don't understand why there's an issue with using margin here? It seems like it should be totally fine to use margin to layout the "insides" of a component? It seems like the main point the author is trying to make is that a "component should not affect anything outside itself." And in this case, it doesn't look like it does? I don't think we should make "avoid margin at all costs" a general rule. On the contrary, I've noticed more issues in our repo where we use padding instead of margin and weirdly extend the "active" clickable areas of elements. |
Comment Actions
@def-au1t It looks like you've missed my comment about performance of searching. It would be great if you could reply to the comments so that the review is more like a conversation - you can either agree or disagree, but having some feedback will improve the experience by a lot.
| web/modals/threads/members/members-modal.react.js | ||
|---|---|---|
| 33 ↗ | (On Diff #10206) | Sorry for late response. I wanted to do some tests and then answer, but I forgot about it. I did some tests how fast the members filtering is on big thread - containing over a hundred users. Even when I slowed down CPU by 6x (using Chrome Dev Tools) this filtering never took more than 1ms (measure with console.time()), with an average time much less: ~0.05ms. It seems, that rendering the list with users take much more time, although it also seems to be enough fast with 100 users. I don't think if there is a need to introduce the index here, but I can do the follow-up task and investigate it if you think it's still worth considering. |
| web/modals/threads/members/members-modal.react.js | ||
|---|---|---|
| 43 ↗ | (On Diff #10527) | Why do we need to check these two conditions? |
| 33 ↗ | (On Diff #10206) | In that case we're fine. About the pagination, if we use that with filtering, it should probably happen server-side, so that might be even slower. We can also render incrementally while the user scrolls, but it doesn't look like a necessity now. |
| web/modals/threads/members/members-modal.react.js | ||
|---|---|---|
| 43 ↗ | (On Diff #10527) | I assumed, that we may consider an admin the user that has admin role (currently only Ashoat user in Genesis) or is an admin of the parent thread (has admin powers), so if any of these conditions is satisfied we put the user in admins group. |
| web/modals/threads/members/members-modal.css | ||
|---|---|---|
| 6 ↗ | (On Diff #10206) |
Anytime we use margin it's affecting a component outside of itself. That's literally the only thing it does: https://developer.mozilla.org/en-US/docs/Web/CSS/margin#description I do:
Using padding or spacer components avoids these two issues entirely. Could we talk IRL where you've seen issues with padding? |
| web/modals/threads/members/members-modal.react.js | ||
|---|---|---|
| 33 ↗ | (On Diff #10206) | Aside from the performance, I think that we probably should change this logic. Could you check what search-index.js does and if it should be used here? It has more complicated logic which might make searching experience better. |
Comment Actions
Thanks for using the existing userStoreSearchIndex, @def-au1t!! And nice suggestion to look into SearchIndex, @palys-swm. Love how thorough / thoughtful the review here has been from everyone