Meter Drop Down #770
Meter Drop Down #770
Conversation
There was a problem hiding this comment.
My thanks to the team for working on this issue and submitting this code. The comments touch on a number of items that need to be addressed. Even if they were addressed, there would remain the issue that the way the meter and group dropdown menus work need to be made constent. Given this and the fact that your work on the project is coming to an end, it might make more sense for someone else to take on making the changes. Would that be okay with the team? Again, your work is appreciated and we would move forward from that if someone else continues this effort.
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
We don't put VSC setting in the repository except as needed by everyone (such as the container we are now using). While removing trailing whitespaces is generally nice, it would not be if it did it to some files such as markdown. Since we now run ESLint in VSC you should see warning about this. Thus, I think we should remove this change.
|
|
||
| if (state.currentUser.profile?.role == 'admin') { | ||
| // Can see all meters that don't have null for unit | ||
| visibleMeters.forEach(meter => { meter.id = Meter.getUnitNotNull }) |
There was a problem hiding this comment.
First, visibleMeters was just created so I think it is an empty set. Thus, doing a forEach would seem to iterate over no items. Second, getUnitNotNull is a database function and not an attribute of Meters. Third, comparing the id to the Meter attribute does not seem correct. Thus, this call does not do what I think you want. The same applies to the else clause.
Since all meters are in Redux state, I think it would be best to get the meter info from that and filter appropriately where you add the meter id for each one desired. I think the group dropdown does that.
| // Also, if not admin, then meters not displayable are not viewable. | ||
| // admin can see all except if unit is null (not included in ones gotten above). | ||
| visibleMeters.forEach(meter => { | ||
| compatibleMeters.add(meter.displayable); |
There was a problem hiding this comment.
The checks above should have already set visibleMeters to what is appropriate for admin or non-admin users. As such, you can set compatible meters to visibleMeters and get the correct result.
Note that you seem to be adding only the displayable attribute of each meter to coompatibleMeters and not the id.
| } | ||
| // If displayable is false then only admin. | ||
| else { | ||
| state.graph.selectedMeters.forEach(meter => {//for each visibleMeters M |
There was a problem hiding this comment.
The design document specified that the loop was over the visible meters where each iteration called the meter M.
OED prefers that comments be on the line above the code it applies to.
| // If displayable is false then only admin. | ||
| else { | ||
| state.graph.selectedMeters.forEach(meter => {//for each visibleMeters M | ||
| M.add(meter) |
There was a problem hiding this comment.
I'm unclear what the M.add is supposed to do.
| else { | ||
| state.graph.selectedMeters.forEach(meter => {//for each visibleMeters M | ||
| M.add(meter) | ||
| const newUnits = unitsCompatibleWithMeters(M) |
There was a problem hiding this comment.
I'm not sure why you call this newUnits when the design document specified units as the name. While changing the name may not impact the result, it is nice to keep the code in line with that document unless there is a reason to change it. If you had a reason then please let me know.
| state.graph.selectedMeters.forEach(meter => {//for each visibleMeters M | ||
| M.add(meter) | ||
| const newUnits = unitsCompatibleWithMeters(M) | ||
| if (newUnits.has(state.meters.byMeterID[meter].defaultGraphicUnit))//graphicUnit is in units |
There was a problem hiding this comment.
This should be checking if the current graphic unit is in the set. This is checking the default graphic unit for a specific meter. They are not the same.
| //Ready to display meters in menu. Note you display the identifier and not the id. | ||
| //For each compatibleUnit C add name C.identifier to meter menu in alphabetically sorted order in regular font for case 1 | ||
| //ForAdd each incompatibleUnit I add name I.identifier to meter menu in alphabetically sorted order as grayed out and not selectable for case 2 | ||
| const finalMeter: SelectOption[] = []; |
There was a problem hiding this comment.
The way this was done was changed (by me) in groups and it should be adapted for use here. This code has some issues in what it is doing so that is another reason to change it.
| @@ -23,4 +27,69 @@ function mapDispatchToProps(dispatch: Dispatch) { | |||
| }; | |||
| } | |||
|
|
|||
| export function getvisibleMeters(state: State) { | |||
There was a problem hiding this comment.
A couple of items on this:
- I'm not sure the name of this function is the best. The group dropdown group named their function getGroupCompatibilityForDropdown.
- The group team put their function in src/client/app/containers/ChartDataSelectContainer.ts. Whatever is done these two should be similar.
- I'm unclear on where this is hooked up to the actual menu. The group team also modified the ChartDataSelectContainer.ts to do this. Maybe I'm missing something and it is hard to be certain since the correct meters don't seem to be chosen.
|
React hooks have now been integrated into OED. Given this and the status of the PR work, the current plan is to close this PR and move forward with a new one. |
|
This was done in PR #787 so this PR is being closed. |
Description
Added functionality to the meter drop-down meter that follows the algorithm that is posted on the dev docs.
Fixes #[issue]
Worked on the container of the Meterdropdown menu
[MeterDropDown]
Type of change
Checklist
Limitations
A limitation was some strange error on my docker that wouldn't allow seeing my changes.