NCD-672: initial code review #26
Conversation
|
Make sure to add the "doc required" or "doc not required" label. |
aadnekar
added
the
doc not required
There was a problem hiding this comment.
Great improvements!
I really think it's cool to see the xor function in use, think it helps to make the code consice and readable.
We could considering extracting it into it's own file, and then rather importing it in the different use cases. But that's far from important.
Well done! 🙌
src/SidePanel.tsx
Outdated
| onClick={() => { | ||
| console.dir(device); | ||
| // console.dir(device); |
There was a problem hiding this comment.
Why do you not remove the console.log?
There was a problem hiding this comment.
I had actually removed it but for some reason it wasn't committed :/ Doing it now 👍
| logger.debug( | ||
| `Got device ${JSON.stringify(device)} ${JSON.stringify( | ||
| boardRevision | ||
| )}` | ||
| ); | ||
| logger.debug( | ||
| `Device with boardVersion: ${JSON.stringify( | ||
| device?.devkit?.boardVersion | ||
| )}` | ||
| ); |
There was a problem hiding this comment.
If the plan is to keep this, we could consider extracting the logging into a function?
// Define it somewhere:
const logDeviceInfo = (device: Device) => {
logger.debug(
`Got device ${JSON.stringify(device)} ${JSON.stringify(
boardRevision
)}`
);
logger.debug(
`Device with boardVersion: ${JSON.stringify(
device?.devkit?.boardVersion
)}
}
// ... and call it
logDeviceInfo(device);sorry for the formatting, wish Github had autoformatting
Tried to incorporate as much as possible from the code review.