Role category Hierarchies #3603
Conversation
| private processRolesForTreeGrid( | ||
| roles: Omit<HoistRole, 'users' | 'directoryGroups' | 'roles'>[] | ||
| ) { | ||
| const topList = []; |
There was a problem hiding this comment.
I think this method would benefit from some more specific variable names, particularly list and topList, which feel pretty ambiguous to me. Maybe ret or root instead of topList and children instead of list?
| let list = topList, | ||
| id = ''; | ||
| categories.forEach(category => { | ||
| let currCat = list.find(it => it.name === category); |
There was a problem hiding this comment.
Should you also be checking that isGroupRow === true? What if you had a role with the same name as a category?
| autosizeOptions: {mode: 'managed'}, | ||
| emptyText: 'No roles found.', | ||
| colChooserModel: true, | ||
| sortBy: 'name|asc', | ||
| sortBy: ['isGroupRow', 'name'], |
There was a problem hiding this comment.
Is it necessary to sort on isGroupRow? That seems confusing to me
| {field: {name: 'category', type: 'string'}, hidden: true}, | ||
| {field: {name: 'lastUpdated', type: 'date'}, ...Col.dateTime, hidden: true}, | ||
| {field: {name: 'lastUpdatedBy', type: 'string'}, hidden: true}, | ||
| {field: {name: 'notes', type: 'string'}, filterable: false, flex: 1} | ||
| {field: {name: 'notes', type: 'string'}, filterable: false, flex: 1}, | ||
| {field: {name: 'isGroupRow', type: 'bool'}, hidden: true, excludeFromChooser: true} |
There was a problem hiding this comment.
Does this need to be a hidden column? Seems like it could just be a Store Field?
| item: hframe( | ||
| vframe( | ||
| grid({ | ||
| agOptions: { |
There was a problem hiding this comment.
I don't think this is necessary anymore - perhaps left over from old implementation?
|
Hi @Ryanseanlee -- could you catch this up, and indicate if all of Greg's comments have been handled? Would love to merge this. |
Hey yes definitely |
| @@ -38,12 +38,16 @@ export class RoleModel extends HoistModel { | |||
| @observable.ref allRoles: HoistRole[] = []; | |||
| @observable.ref moduleConfig: RoleModuleConfig; | |||
|
|
|||
| @bindable groupingDisabled = false; | |||
There was a problem hiding this comment.
its not that grouping is 'disabled' its that its not actually being grouped at the moment. Would call it -- `showInGroups' or something like that
| @@ -212,6 +227,30 @@ export class RoleModel extends HoistModel { | |||
| }); | |||
| } | |||
|
|
|||
| private processRolesForTreeGrid( | |||
| roles: Omit<HoistRole, 'users' | 'directoryGroups' | 'roles'>[] | |||
There was a problem hiding this comment.
I need someone to explain to me how this works :)
|
@Ryanseanlee -- please let me know when you are ready to go over? Hopefully monday Would be helpful if you could resolve the comments above that you have dealt with -- thanks |
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.