-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
LF-4696: UI to display ensemble sensors list #3686
LF-4696: UI to display ensemble sensors list #3686
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I the only one who gets stressed when finding the right icons? I've been renaming svgs, but I kept it this time since it would be easier if the name stays the same as in figma... (I always struggle with placing icons in the right folder too...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not the only one, the icons folder is a mess 😂 we'd probably need to take a look at what the icon library looks like in FIgma and then organize ours accordingly.
NOTE: I plan to update "ESCI" to "ESci" in translations in another PR for reorganizing folders and files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how this code is organised and the components broken up ❤️ I particularly like the new components OverviewStats and MainContent and I hope we will push back harder on any future designs that create yet more patterns of expandable and/or KPI! It would be nice to instead see these again elsewhere in app.
(Note: the Styles appear off in Storybook due to the known issues)
That is this, right?


I think I usually just over-specify the selectors in the SCSS (e.g. the yes/no buttons could be nested under their container) until it also works in Storybook -- is that a bad habit?
Edit:
One thing that is not at all blocking for this PR (it's really for Loïc, but I want to remind ourselves to check next week)...

This was the design based upon the two sensors Loïc saw on the Ensemble website. When he asked Morten if these were the only two possibilities, Morten said "no" and replied with the list of all sensor types. I don't think the designs got updated subsequently and we should circle back on this. I'm pretty sure all the possible sensor types should be allowable in KPI. The code is very extensible here (🙏 ) so the adjusts can definitely be made after merge.
@@ -1826,6 +1834,7 @@ | |||
"MONTHS_AGO": "{{time}} month(s) ago", | |||
"NO_DATA": "(no data)", | |||
"NO_DATA_FOUND": "No sensor readings found", | |||
"PARTNER_SENSOR_LIST": "{{partner}} sensor list", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't know if this might be fundamentally the better solution (to interpolate 'Ensemble' and 'ESci' in English wherever they occur), but I think what we decided on Wednesday was that it should be part of the string so it can be transliterated into different scripts if desired by the translators.
At the same time I had forgotten that the loading screen also interpolates {{dataName}}
so right now we're mixed. I think I would prefer to stick to either one or the other throughout 🤔 Maybe @antsgar has an opinion on that?
packages/webapp/src/components/Sensor/v2/EsciSensorList/index.tsx
Outdated
Show resolved
Hide resolved
packages/webapp/src/containers/LocationDetails/PointDetails/SensorDetail/v2/constants.ts
Outdated
Show resolved
Hide resolved
packages/webapp/src/stories/ExpandableItem/ExpandableItem.stories.jsx
Outdated
Show resolved
Hide resolved
} | ||
|
||
.inlineRemoveWarning, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious -- is it because I used grid
in the other one that it's not possible to extract a shared visual component between this and AnimalFormHeaderItem
? Or is it just more trouble than it's worth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this a long time ago, so I don’t fully remember what I did…😅 But I think AnimalFormHeaderItem
is more complex than what this reusable component can cover, so I likely treated them as separate things. Was there a specific part where you saw a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, not a difference but I just saw the chunk of code wrapped by the div with .inlineRemoveWarning
was so close to duplicated I thought maybe you would have tried to extract it first and maybe that didn't work out? But it's not so much code that it matters.
Yes unfortunately AnimalFormHeaderItem
is different enough again such that I don't think it could ever use MainContent
😜 So many expandables!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you were referring to inlineRemoveWarning
specifically! (It's so obvious looking back 😂)
I think I just got lazy and didn’t create a new component, which makes no sense... 🙄
I'll clean that up in the next PR 🙏
Co-authored-by: Joyce Yuki <82857964+kathyavini@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Joyce for reviewing and for checking unused ones as well 🙏
I think I addressed all comments and also pushed a missed commit for OverviewStats
styles, along with a small improvement to MainContent
(making errorCount
optional).
I always want to nest CSS! It's more readable than comments... It fixed the styling in Storybook perfectly, thank you!
I spent too much time accommodating the unknowns in the KPI, so I'm hoping we'll just be adding more types without many adjustments 🤞 I'll check with Loïc next week!
} | ||
|
||
.inlineRemoveWarning, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this a long time ago, so I don’t fully remember what I did…😅 But I think AnimalFormHeaderItem
is more complex than what this reusable component can cover, so I likely treated them as separate things. Was there a specific part where you saw a difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I love that with MainContent having its own set of stories 😍
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much code here and yet really readable and easy to follow, great work @SayakaOno! Unfortunately I wasn't able to see it come to life because the Ensemble API is currently erroring out but the code looks great 👏
const formatKpiLabel: OverviewStatsProps['format'] = (statsKey, label) => { | ||
const Icon = statsKey === SENSOR_ARRAY ? SensorArrayIcon : SensorIcon; | ||
return ( | ||
<div className={styles.kpiLabel}> | ||
<span className={styles.iconWrapper}> | ||
<Icon /> | ||
</span> | ||
<span className={styles.text}>{label}</span> | ||
</div> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be a component instead of a plain function, since it returns a React node?
const formatKpiLabel: OverviewStatsProps['format'] = (statsKey, label) => { | |
const Icon = statsKey === SENSOR_ARRAY ? SensorArrayIcon : SensorIcon; | |
return ( | |
<div className={styles.kpiLabel}> | |
<span className={styles.iconWrapper}> | |
<Icon /> | |
</span> | |
<span className={styles.text}>{label}</span> | |
</div> | |
); | |
}; | |
const FormatKpiLabel: OverviewStatsProps['format'] = (statsKey, label) => { | |
const Icon = statsKey === SENSOR_ARRAY ? SensorArrayIcon : SensorIcon; | |
return ( | |
<div className={styles.kpiLabel}> | |
<span className={styles.iconWrapper}> | |
<Icon /> | |
</span> | |
<span className={styles.text}>{label}</span> | |
</div> | |
); | |
}; |
and then in the OverviewStats component
<span className={styles.category}>{FormattedLabelComponent ? <FormattedLabelComponent key={key} label={label} /> : label}</span>
format={formatKpiLabel} | ||
isCompact={isCompact} | ||
/> | ||
<div className={styles.sensorGroups}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be ul
instead of div
since it's a list? We're probably defaulting to divs a bit too much in general and should try to use more semantic tags if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, and this is definitely a list!
It wouldn't be a quick change since ExpandableItem
would need the ability to render li
and div
conditionally, but we could create a ticket to update it!
type SensorsMapKeys = SensorArray['id'] | typeof STANDALONE; | ||
type MappedSensors = { [key: SensorsMapKeys]: SensorInSimpleTableFormat[] }; | ||
|
||
const formatSenorArrayToGroup = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const formatSenorArrayToGroup = ( | |
const formatSensorArrayToGroup = ( |
summary: SensorSummary; | ||
}; | ||
|
||
const EsciSensorList = ({ groupedSensors, summary }: EsciSensorListProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, do you think this component is specific to ESci and would not be applicable if we were to integrate another partner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m hoping we can make this component reusable for another partner in the future with minimal changes. The title and sensor table header are currently ESci specific, making the component non-reusable, so I named it accordingly!
Thank you @antsgar for reviewing and suggestions! @kathyavini I think I've addressed Anto's comments, it would be appreciated if you could review the changes. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thank you for the reminder! I wouldn’t want to ask you for another re-review 😂, so I'll include it in my next PR! |
Description
MainContent
component (usable inExpandableItem
)simple
variant storydecorated
variant storyWithStepperProgressBar
getValues()
as an argument toonAfterSave
OverviewStats
component (will be adjusted later if necessary)PageTitle
prop typeEsciSensors
componentSensorTable
componentCell
components for better reusabilityTableHeader
className
SensorList
containeruseGroupedSensors
hook/sensors?partner_id={partnerId}
Jira link: https://lite-farm.atlassian.net/browse/LF-4696
Type of change
How Has This Been Tested?
Checklist: