Open NicolaLS opened 3 months ago
addressed everything, PTAL @thisconnect :)
componentDidMount and componentWillUnmount are replaced by this.
I believe "this" and a few other links broke. also it's hard to follow please consider using less links or paste full links. but if you rebase / squash etc. such links might break.
componentDidMount and componentWillUnmount are replaced by this.
I believe "this" and a few other links broke. also it's hard to follow please consider using less links or paste full links. but if you rebase / squash etc. such links might break.
ah I see..I think I will just remove the note now, thanks for letting me know. it was not super important just meant as a quick overview before starting the review but you already did that. (the links just pointed to the respective useEffect
s..)
tested b95ae02c0b9347e753ed643957dfb8fbd06e39a7 webdev/servewallet and initChart is called, but somehow the chart isn't loading.
tested b95ae02 webdev/servewallet and initChart is called, but somehow the chart isn't loading.
![]()
actually, I forgot testing after removing the useEffect
that calls initChart
if !chart.current
..but I tested right now without changing anything and webdev/servewallet
seem to work fine for me on firefox and chrome..I'll look into this though.
(rebase master force push )
tested b95ae02 webdev/servewallet and initChart is called, but somehow the chart isn't loading.
![]()
@thisconnect so it is definitely the useEffect
I removed, I added it back in and the chart shows again. I would argue that for a transformation of the code it is also correct that it is in there, since the original code also contains:
public componentDidUpdate(prev: Props) {
const { chartDataDaily, chartDataHourly } = this.props.data;
if (!this.chart) {
this.createChart();
}
which translates to the useEffect
that runs every render. I agree that it should not be like this, but would it make sense to refactor this in another PR? Of course I can do it in this PR too but I think it would be better in another PR.
Do you have any env variables for react? Maybe the chart shows for me because react runs useEffect
twice in dev mode (so you're not in dev mode?)
found a weird issue, something seems wrong:
* click week * rotate to BTC * white screen & js error
![]()
ah yes, this also happens when clicking on some account now and then going back to the account overview..its weird because I'm pretty sure that this was not an issue and some point, I don't know what change caused this (maybe it was an issue, I'm not hundred percent sure)
the problem is we set the state of chartDisplay
and source
when the switch in initChart
matches anything (this is why there is no issue if 'all' is selected. this causes the chart.current
to stay uninitialized causing the infinite loop.
I will just fix the chart initialization now, thought it would be better in another PR but it seems like I can only get this one to work with a proper chart initialization logic.
I changed everything in the first commit but it stays broken with the issue you described here
I will add another commit fixing the initialization, but only for a easy review, the commits need to be squashed since the first one is broken
@thisconnect all change requests should be fixed/outdated now, I did not respond to all the comments individually to create less noise. PTAL or resolve the threads that are satisfied in case there is more to do (thanks :))
Convert the Chart class component in chart.tsx to a functional component.
There some general improvements that could be made, but this PR aims to perform minimal changes to only transform the component from a class component to a functional one. I will open another PR with a general refactor when this one was merged.
note: I moved the
renderDate
function out of the component, but this improvement should've been in the next PR not this one, lmk if I should put it back.