-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LF-4699(1): Display sensor readings on the UI #3714
base: integration
Are you sure you want to change the base?
Conversation
* handle time zone, correct logic
* extract functions * adjust xAxis padding
874f2d7
to
45613cf
Compare
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.
The charts look amazing! ❤️ I left a couple of small comments, but this looks great. I assume there'll be a follow up PR for showing these on the sensor detail view, correct?
'--Colors-Accent---singles-Blue-full': '#0669e1', | ||
'--Colors-Accent---singles-Red-full': '#d02620', | ||
'--Colors-Accent-Accent-yellow-600': '#e8a700', | ||
'--Colors-Primary-Primary-teal-700': '#266f68', | ||
'--Colors-Accent---singles-Purple-full': '#8f26f0', | ||
'--Colors-Accent---singles-Brown-full': '#aa5f04', |
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 we follow the format of the other colors in the theme here instead of the "CSS variable" format? So something like accentYellow600
? I also wonder if these colors are only going to be used for charts, in which case it might be good to make that explicit in the naming and call them something like chartBlue
, chartRed
. Perhaps we could check that with Loïc?
const CustomTooltip = ({ active, payload, label }: TooltipProps<ValueType, NameType>) => { | ||
if (active && payload?.length && activeLine) { | ||
const data = payload.find(({ dataKey }) => dataKey === activeLine); | ||
|
||
if (data) { | ||
const { color } = lineConfig.find(({ id }) => id === activeLine)!; | ||
const xAxisData = isTimeScaleProps(props) | ||
? getDateTime(label, props.language, props.truncPeriod, t) | ||
: label; | ||
const value = formatTooltipValue?.(label, data.value) || data.value; | ||
|
||
return ( | ||
<dl className={styles.tooltip} style={{ '--tooltipColor': color } as React.CSSProperties}> | ||
<dt>{xAxisData}</dt> | ||
<dd>{value}</dd> | ||
</dl> | ||
); | ||
} | ||
} | ||
|
||
return null; | ||
}; |
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.
Could we move this component outside of LineChart
and pass any props needed from LineChart to it? It's best not to declare a component inside another one, because it will be re-created on every render. I also think it's harder to figure out which data the component needs to function when it's declared inside another one and uses some of its props/state
Description
LineChart
component and stories.getTicks
function and tests (pnpm test chartUtils
)Jira link: https://lite-farm.atlassian.net/browse/LF-4699
Type of change
How Has This Been Tested?
Checklist: