Refactor watchers functionality #610
Conversation
anru
force-pushed
the
ashes/get-rid-of-store-creator
branch
from
9fa6d01 to
6c092fc
Compare
… actions instead of having just the same only because there was too much complicated state & connect state to components logic
|
To reviewers: you don't have to review all changes here. You can just test final functionality for example. But if you want .. you are welcome 😈 |
| } | ||
|
|
||
| get isParticipantsLoading(): boolean { | ||
| return _.get(this.props.asyncActions, 'fetchParticipants.inProgress', false); |
There was a problem hiding this comment.
extract to mapStateToProps?
| }; | ||
|
|
||
| renderCell(user, hidden = false) { | ||
| const { props } = this; |
There was a problem hiding this comment.
not needed here as just on props usage below
|
|
||
| get usersRow() { | ||
| const { props } = this; | ||
| const users = this.props.participants; |
There was a problem hiding this comment.
this.props not needed as destructured above ^
| }; | ||
|
|
||
| get usersRow() { | ||
| const { props } = this; |
There was a problem hiding this comment.
maybe better destructure to single values?
const { participants , emptyTitle, maxDisplayed, ... } = this.props;
There was a problem hiding this comment.
No. It's not better. Also I found this approach a little bit ugly and non informative.
It's better then you have clear context where are from this variable or that. It's first point.
Second point is, actually, when you have a lot of variables you have more letters than you might have.
|
|
||
| @autobind | ||
| handleAddClick() { | ||
| const { props } = this; |
| } | ||
|
|
||
| get usersBlock() { | ||
| const { props } = this; |
|
|
||
| const entityTitle = numberize(entity.entityType, 1); | ||
| switch (group) { | ||
| case 'watchers': |
There was a problem hiding this comment.
i think it's better to extract constants somewhere in paragon or smth
| className="fc-add-watcher-modal" | ||
| > | ||
| <div className="fc-modal-body fc-add-watcher-modal__content"> | ||
| <AdminsTypeahead |
There was a problem hiding this comment.
why does SelectUsersModal uses AdminsTypeahead?
is it used to work with admin users only? if so, would be better to rename to SelectAdminsModal
| @@ -12,11 +12,19 @@ import ContentBox from '../content-box/content-box'; | |||
|
|
|||
| @@ -0,0 +1,8 @@ | |||
|
|
|||
| type UserType = { | |||
There was a problem hiding this comment.
Type is used to prevent name collisions?
what if start using T prefix?
lile TUser
also add numberize for `users` word
|
@tonypizzicato I fixed regression |
|
LGTM! |
this PR doesn't add or remove functionality, just refactoring.
WatherTypeaheadcomponent into 2 new:UsersTypeaheadandAdminsTypeahead, second one is independent container which connect "searching admins" functionality to dumbUsersTypeaheadcomponentmakeWatchersfunction for every entity in the system