Skip to content

Conversation

xplato
Copy link
Contributor

@xplato xplato commented Apr 30, 2024

Adds read-only NoiseSensorDeviceDetails component.

Screenshot 2024-05-01 at 12 01 31 PM

@xplato xplato marked this pull request as ready for review May 1, 2024 18:13
@xplato xplato requested review from razor-x, mikewuu and dawnho as code owners May 1, 2024 18:13
<DetailSection
label={t.noiseThresholds}
tooltipContent={
device.device_type === 'minut_sensor' ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does minut have special behavior here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export function NoiseThresholdsList({
device,
}: NoiseThresholdsListProps): JSX.Element {
const { noiseThresholds, isInitialLoading } = useNoiseThresholds({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we passing isInitialLoading around instead of showing a loading state for the entire section?

Are we using or can we use the loading behavior / loading toasts like we do elsewhere? It's ok if we address this as a follow up PR, but I thought we had standardized the component loading behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid an additional ternary operator on Content. But that's a good point, I can add the loading toast as well. There's no design for this, and it will overlay the device card at the top. I'll address this in a subsequent PR if you don't mind.

Co-authored-by: Evan Sosenko <evan@getseam.com>
@xplato xplato merged commit b54f438 into main May 2, 2024
@xplato xplato deleted the noise-sensors branch May 2, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants