Firefly-2024: New MOC features in DCE and code clean up - #1995
Conversation
robyww
changed the title
d7b6b8a to
2231fbf
Compare
jaladh-singhal
left a comment
jaladh-singhal
left a comment
There was a problem hiding this comment.
I tested the builds, working as described. @lrebull can test in depth when she's back.
I left some minor code improvement comments, otherwise good to go.
| @@ -1,4 +1,6 @@ | |||
| import {get, set, isEmpty, flatten, isArray} from 'lodash'; | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
I don't see the use of React import in this file. Also it's a .js not .jsx so either we should remove it or change file extension?
|
|
||
| if (sd?.serDefParams) { | ||
| for (const param of sd.serDefParams) { | ||
| productUrl.inputParams[param?.name] = {value: param.value, ref: param.ref}; |
There was a problem hiding this comment.
You may wanna check param?.name before doing assignment otherwise there will be a "undefined" key in inputParams. I've noticed this issue in some of our other code as well.
There was a problem hiding this comment.
I will put an if around it.
| targetPanelExampleRow2: examples.slice(Math.trunc(cnt / 2)) | ||
| }; | ||
| } else { | ||
| return {targetPanelExampleRow1: [inExample], targetPanelExampleRow2: []}; |
There was a problem hiding this comment.
This probably out of scope since it will change data structure everywhere else but instead of breaking examples to two rows we can let CSS handle that during rendering based on line wrapping.
There was a problem hiding this comment.
That could be really complex and often they want more control. It is something we could look at in the future.
| .reduce((obj, {name, value, desc}) => { | ||
| const key = itemsList.find((k) => name.startsWith(k)); | ||
| if (key) { | ||
| obj[translateXml[key] + name.substring(key.length, name.length)] = value; | ||
| } else { | ||
| obj[name] = value; | ||
| obj['mocTitle' + name.substring(3, name.length)] = desc; | ||
| } | ||
| return obj; | ||
| }, {}); | ||
| } | ||
|
|
||
| function getMOCList(searchAreaInfo) { | ||
| const mocAry= Object.keys(searchAreaInfo) | ||
| .filter( (k) => k.toLowerCase()==='moc' || k.match(/moc\d+$/i) ) | ||
| .map( (k) => { | ||
| const cnt= k.substring(3,k.length); | ||
| return { | ||
| mocUrl : searchAreaInfo[k], | ||
| title : searchAreaInfo['mocTitle'+cnt] ?? 'MOC'+cnt, | ||
| [SHORT_TITLE]: searchAreaInfo[SHORT_TITLE+cnt], | ||
| [MOC_COLOR]: searchAreaInfo[MOC_COLOR+cnt], | ||
| maxFetchDepth: searchAreaInfo['maxFetchDepth'+cnt], | ||
| [MOC_STYLE]: searchAreaInfo[MOC_STYLE+cnt], | ||
| }; |
There was a problem hiding this comment.
I think this code can be improved:
- You can omit second argument in substring - it will default to string.length instead of you specifying it again and again
- There seems to be some string parsing and reverse parsing association between these two functions for object keys e.g.
cnt= k.substring(3,k.length); searchAreaInfo['mocTitle'+cnt]andobj['mocTitle' + name.substring(3, name.length)] = desc;. It might be worth abstracting those associations out to helper functions so that they don't go out of sync and are more readable.
(I recommend feeding it to AI, it's usually good at this kind of things)
There was a problem hiding this comment.
This processing was happening at two different places in the code. I realize it could be combined and happen very early.
I will look at cleaning it up some more.
- includes response to feedback support: <PARAM name="moc_style" datatype="char" arraysize="*" value="outline"/> <PARAM name="moc_short_description" datatype="char" arraysize="*" value="herschel_pep"/>
2231fbf to
7cb06cd
Compare
2 participants
Firefly-2024: new moc features in DCE and code clean up
CisxSerDescUtils.jsand moved some functions to it.Testing
dustpediaTest File
dustpedia.xml