Redesign change user experience modal #3077
Conversation
…s is now fully clearable; added button next to input to clear it
… now ordered but case-sensitive
…ctionality is still missing; fixed table displaying errors
added submit functionality to modal that changes the experience of multiple users and enabled negative input to add possibility of shared decrease in exp domains
…ing mutiple users
…l component; added remove feature of entries to the multiple users edit table
…es the returned promises
MichaelBuessemeyer
changed the title
There was a problem hiding this comment.
Very nice work 👍 I see that you put a lot of effort into this PR and it already feels like a huge improvement over the old dialog 🎉
However, there are some things which we need to get done before merging, I think:
- As far as I understand, it's currently not possible to not overwrite "varying" experience domains when editing multiple users. Is this correct? I'd suggest to leave the input fields empty for such cases and simply not touch these domains if the user didn't input any change.
- I managed to break the dialog several times (whole page turned white). I think I did the following: select multiple users for editing, change some domain, click revert and then click delete. I think I also found the reason for this (see my review comments for that).
- Code wise, I made some comments which you can just follow. The only "bigger" thing is that there are a lot if if-else branches for the single vs. multiple mode. Ideally, we would avoid those. I'd hope that the multiple mode could just cover the single mode so that we don't need to handle the single mode as an extra case. Do you think that's possible?
| this.setState(prevState => ({ | ||
| isExperienceModalVisible: false, | ||
| users: prevState.users.map( | ||
| user => updatedUsers.find(currentUser => currentUser.id === user.id) || user, |
There was a problem hiding this comment.
For the sake of performance, I'd replace the find method with something that has a near-constant look-up time. You can construct an object which maps from user id to the corresponding updated user with const updatedUsersMap = _.keyBy(updatedUsers, u => u.id). Then, you can replace the find-call with updatedUsersMap[user.id]. The code would then look like this:
this.setState(prevState => {
const updatedUsersMap = _.keyBy(updatedUsers, u => u.id);
return {
isExperienceModalVisible: false,
users: prevState.users.map(user => updatedUsersMap[user.id] || user)
};
});(I didn't test the code, so please incorporate it with care :))
| this.setState({ domains: await getExistingExperienceDomains() }); | ||
| } | ||
|
|
||
| removeAlreadyUsedDomains(): ExperienceDomainListType { |
There was a problem hiding this comment.
Please rename this method to getUnusedDomains. The current name suggests that data is mutated, but this is not the case :)
|
|
||
| const { Column } = Table; | ||
|
|
||
| // TODO => visual bug when editing a value of a domain the first time !!! |
| title="Current Experience Value" | ||
| width="30%" | ||
| render={(record: MultipleUserTableEntry) => | ||
| // eslint-disable-next-line no-nested-ternary |
There was a problem hiding this comment.
Eslint rules should only be disabled for a good reason :) Avoiding nested ternaries improves readability and could look like this:
if (record.highestValue === -1 && record.lowestValue === -1) {
return "";
}
return record.highestValue === record.lowestValue ? record.highestValue : `varying ...`;
| className="experience-change-modal" | ||
| title={ | ||
| mutlipleUsers | ||
| ? "Changes Experiences of Multiple Users" |
There was a problem hiding this comment.
I'd change this to ... of Selected Users.
| updatedAllUsers = async () => { | ||
| let newUserPromises: Array<Promise<APIUserType>>; | ||
| if (this.state.showOnlySharedExperiences && this.props.selectedUsers.length > 1) { | ||
| const relevantEntries = this.state.multipleUsersEntries.filter(entry => entry.isShared); |
There was a problem hiding this comment.
I'd rename this to sharedEntries since this is clearer.
| const relevantEntries = this.state.multipleUsersEntries.filter(entry => entry.isShared); | ||
| newUserPromises = this.props.selectedUsers.map(user => { | ||
| const newExperiences = { ...user.experiences }; | ||
| relevantEntries.forEach(entry => { |
There was a problem hiding this comment.
Here, you are initializing newExperiences and then you update it in a forEach call. In general, it's nicer if you can define a variable directly with the right values so that you don't need any mutation. In this case, you can do something like this:
const newExperiences = {
...user.experiences,
..._.fromPairs(relevantEntries.map({domain, value} => [domain, value]))
};
_.fromPairs takes something like [["a", "b"], ["c", "d"]] and returns {a: "b", c: "d"}. With the ... spread, the new experiences are directly merged into the existing ones.
| }); | ||
| // sort entries | ||
| return sharedTableEntries.sort( | ||
| Utils.localeCompareBy(([]: Array<MultipleUserTableEntry>), entry => |
There was a problem hiding this comment.
This sort code appears four times in this module. Can you extract this into a helper method so that you only need to write sort(sharedTableEntries)?
| } | ||
| }); | ||
| // create sharedTableEntries | ||
| sharedTableEntries = sharedTableEntries.map(entry => { |
There was a problem hiding this comment.
Why is the variable named sharedTableEntries if it can contain entries with isShared == false? (line 95)
|
|
||
| const newUser = update(user, { | ||
| experiences: { [domain]: { $set: newExperienceLevel } }, | ||
| loadMultipleUserEntries = (users: Array<APIUserType>): Array<MultipleUserTableEntry> => { |
There was a problem hiding this comment.
I find this method a bit hard to understand. Can you try to make more use of lodash's helper functions? For example, if you want to find out which experience domains are shared you can write something similar to:
_.intersection(...users.map(user => Object.keys(user.experiences)))
When you want to find out the min/max value for a certain domain you can leverage _.min/_.max etc.
…elete domains, they are also displayed to the user
|
@philippotto The code should be working now and ready for another review. I could not find a unwanted behaviour or bug in it 😃 |
| domain, | ||
| value, | ||
| lowestValue: -1, | ||
| highestValue: -1, |
There was a problem hiding this comment.
lowestValue and highestValue are probably not used for the "single-user mode", but wouldn't it make more sense to write value into these variables?
There was a problem hiding this comment.
But -1 probably means that the interval is not rendered in the table, right? If you add the comments to value, lowestValue, highestValue which I mentioned somewhere else, this will probably clear it up for me :)
|
|
||
| this.closeModal(newUserPromises); | ||
| loadTableEntries = (users: Array<APIUserType>): Array<TableEntry> => { |
There was a problem hiding this comment.
I'd avoid the word load here, since it sounds as if this data is fetched from the server (that's why functions starting with load usually return a promise). How about aggregateTableEntries or just getTableEntries?
| const allSharedDomains: Array<string> = _.intersection( | ||
| ...users.map(user => Object.keys(user.experiences)), | ||
| ); | ||
| const allDomains: Array<string> = _.union(...users.map(user => Object.keys(user.experiences))); |
| updateAllUsers = async () => { | ||
| let relevantEntries = this.state.tableEntries.filter( | ||
| entry => | ||
| entry.changed || (entry.lowestValue === entry.highestValue && entry.highestValue >= 0), |
There was a problem hiding this comment.
Why is the second condition (low === high && high >= 0) necessary? Shouldn't the changed flag be enough to decide whether the entry is "relevant"?
There was a problem hiding this comment.
You are correct, this actually produces a bug 😅 => resolved
| if (!this.props.visible) { | ||
| return null; | ||
| setValueOfEntry = (index: number, value: number) => { | ||
| if (value >= 0) { |
There was a problem hiding this comment.
If you invert the condition here, you can have a flow like this:
if (value < 0) {
return;
}
this.setState...
This removes one indention level from the function and makes it easier to read :)
| visible={this.props.visible} | ||
| onCancel={this.props.onCancel} | ||
| width={600} | ||
| maskClosable={false} |
There was a problem hiding this comment.
The default for maskClosable is false, which is why you can remove this line.
|
|
||
| type TableEntry = { | ||
| domain: string, | ||
| value: number, |
There was a problem hiding this comment.
Can you add a comment here which explains in which cases value, lowestValue and highestValue are -1? This variable denotes a special meaning, right? It's not yet clear to me, when -1 is used, so it would be cool to explain it here.
|
Great 🎉 I also asked the lab for some feedback and in general they liked it pretty good. The following two minor requests came up:
Also I have a suggestion for renaming the table headers:
|
|
@philippotto Adding the number to the
|
|
@philippotto This PR should be ready for another round 🙂 |
… with explaining tooltips
Nice, this looks and feels really good 👍 I'm very happy with how this turned out.
Yes, this is a good idea! Since I wasn't quite sure what would "feel good" in this case, I fiddled a bit around with the code (and also committed it, since I was already at it) and went for a colorful badge, which is green if the domain is shared (otherwise it's white). The badge also has a tooltip explaining what's going on. What do you think? Is this easy to understand from a user perspective? If so, I think this is ready to land 🎉 Looks like this: |
In this PR the modal to change the experience of one or multiple users got a rework.
Now there is a difference between editing multiple users' and a single user's experience.
single user
muliple users
-Submitting the entered changes will result in the changes described in the third point.
general:
After submitting the changes the user-list-view should not fetch the users' data again. Instead it should use the result of the backend send by changing the experiences of a user.