Unify charts #368
Unify charts #368
Conversation
| chartPeriod, | ||
| classes, | ||
| }: CombinedChartsProps) => { | ||
| const [open, setOpen] = useState<boolean>(false); |
There was a problem hiding this comment.
Type could be implicit, but this is fine too.
| <CircularProgress size="6rem" thickness={1} /> | ||
| </Box> | ||
| ) : ( | ||
| (spotterData && spotterData.bottomTemperature.length > 1 && ( |
There was a problem hiding this comment.
Optional chaining might work here?
There was a problem hiding this comment.
I thought so too, but spotterData?.bottomTemperature.length gives an Object is possibly 'undefined' error.
There was a problem hiding this comment.
Ah I see, its because you cant compare undefined which could be returned by an optional chain with a number according to TS. We could leave this then 👍
|
Maybe this component could be moved to |
| const [open, setOpen] = useState<boolean>(false); | ||
| const spotterDataLoading = useSelector(reefSpotterDataLoadingSelector); | ||
|
|
||
| const spotterComponent = useCallback(() => { |
There was a problem hiding this comment.
When I mentioned simplifying the logic here, I was moreso thinking of having 3 booleans with simple, expressive names, so we know the 3 states this conditional can be in just by reading them.
For example:
const doShowLoading=....
const doShowSpotterData=...
const doShow...=....
then when rendering:
{doShowLoading &&<.../>}
{doShow... && <.../>}
etc.
So we don't have to do the logic in our heads when we read this haha.
I'm not sure about this callback however, as when I see this hook I think its a utility function, not a component.
If you think a component is best, maybe moving it outside and making it a proper component with its own props and everything would be nice. However if you think this would be a mess (there are quite a lot of props you'd have to pass down) my approach might work well too, you'll just have to think of some good logic for the booleans.
There was a problem hiding this comment.
For the boolean approach, since the components are mutually exclusive, you might need to add extra logic to those 3 booleans to make sure only one ever will be true. Otherwise, if thats too complicated, you could just use the logic you have right now for the 3 booleans then do:
doShow... ? <.../> : doShow... ? <.../> : <../> Its a nested ternary, but since the boolean names make it easy to read, I think it won't be an issue :) Eric can weigh in on this.
There was a problem hiding this comment.
@tleraj After a second look, I think we can just remove the useCallback and just rename spotterComponent to something more descriptive and in PascalCase so we can wrap this PR up 👍
useCallback would be perfect here if we were passing this component as a dependancy, but in its current state useCallback would likely actually worsen performance than improve it.
Doing this, and adding comments for the 3 if statements inside would be perfect!
There was a problem hiding this comment.
@SimplyBallistic So I decided to use the boolean logic you mentioned above. I used the names spotterDataLoading, spotterDataSuccess and spotterDataError. Let me know if they sound good 🙂
There was a problem hiding this comment.
Looks good! I would just add a prefix to the boolean names like *is*SpotterDataLoading
This PR is scheduled to fix #354.