Section Setup: req 5 - customize "secrets" wording based on login type #34599
Conversation
There was a problem hiding this comment.
Im requesting changes for unit tests (the style comment I wrote is purely for your consideration, since I think the code would look good either way).
For this diff, the code doesn't have a lot of branches so I could see an argument for not adding tests, but even so I'd add them if I were you. I think it'd save you time on manual verification and would help build confidence for future people making changes to this file that their changes didn't break any existing functionality, without them having to do manual verification of that.
Happy to discuss further if you disagree
| @@ -210,6 +210,41 @@ class ManageStudentsTable extends Component { | |||
|
|
|||
| // Cell formatters. | |||
|
|
|||
| passwordHeaderFormatter = () => { | |||
| const {loginType} = this.props; | |||
| const passwordLabels = {}; | |||
There was a problem hiding this comment.
Nice to see you using hashmaps for this sort of thing!
nit - this is a totally subjective, nonblocking comment. I think this code looks great as is and I'd like it without any changes, but I think it'd be a little bit more readable if you initialized the values for passwordLabels and passwordTooltips for each key side by side, like this:
const passwordLabels = {}
const passwordTooltips = {}
passwordLabels[SectionLoginType.picture] = i18n.picturePassword()
passwordTooltips[SectionLoginType.picture] = i18n.editSectionLoginTypePicDesc()
passwordLabels[SectionLoginType.word] = i18n.secretWords()
passwordTooltips[SectionLoginType.word] = i18n.editSectionLoginTypeWordDesc()
... etc ...
This makes it slightly easier to see that passwordTooltips and passwordLabels each have a value for each SectionLoginType, and to edit multiple values for any individual key if you ever have the need to. It'd also make it easier to add new metadata values for a new key, since doing this would require less scrolling - everything would just go into one contiguous block of new code rather than in several sections of code.
In this case since there are only three SectionLoginTypes and only two categories of metadata you are defining for each what you have is about as readable, but if there were more loginTypes or more categories of metadata I think grouping the metadata values by key would be a bit more noticeably clear. I think its always a good practice to expect code to become more complicated over time, and to code accordingly, (in this case, to expect that we may add new categories or logintypes later).
Again, this is all subjective and I think what you have looks great - just thought I'd share these thoughts.
LP-1391
Spec
Instead of titling the password column 'secrets' for all login types, the column header is now customized based on login type and has a relevant explanatory tooltip. Additionally, the show/hide secret buttons in password column for a given student are now customized for word and picture sections. OAuth login types do not have a password column in the Manage Students table.
Word:
Picture:
Personal email: