-
Notifications
You must be signed in to change notification settings - Fork 82
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
frontend: convert chart.tsx component #2728
base: master
Are you sure you want to change the base?
frontend: convert chart.tsx component #2728
Conversation
ec9760f
to
d7e667c
Compare
addressed everything, PTAL @thisconnect :) |
|
||
private onResize = () => { | ||
const isMobile = window.innerWidth <= 640; | ||
this.setState({ isMobile }); |
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.
why isn't isMobile updated anymore on resize?
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.
because it is calculated every time the component renders now. see here.
edit: ah, the component does not re-render automatically when the window re-sizes..good catch. I will convert isMobile
to state again, and initialize it just like right now, but also setIsMobile
in onResize
then.
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.
can you past full links instead of "here", in some other comment you have like 3 links "here" "here" "this" this is hard for me to read.
also this should not update on ever render but on resize.
this.lineSeries.setData(data); | ||
this.chart?.timeScale().fitContent(); | ||
this.setFormattedData(data); | ||
}); |
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 believe this effect is not needed, as the previous effect already and unconditionally calls initChart().
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.
yes, I also thought that, but I thought I should put it in there since there was this.
So yes logically it does not need to be there but if we want to strictly say we only want to convert the component it should be there? But I'll remove it and test if everything still works.
chart.current.unsubscribeCrosshairMove(handleCrosshair); | ||
} | ||
}; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
is this needed?
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.
we explicitly want to run the effect once which is possible by passing an empty dependency array according to the react docs. however eslint wants us to either add the used dependencies, or remove the dependency array (which would cause it to run every render).
so we either ignore the warning with the comment or remove the dependency array but check if it already ran once e.g
const chartInitialized = useRef(false);
useEffect..
if(!chartInitialized) ...chartInitialized = true, initChart() return cleanup
else do nothing
so the function in use effect would return () => void | undefined
not sure if thats even possible ? can it have no cleanup function when some condition is not met?
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.
tldr; yes otherwise we get eslint errors
d7e667c
to
861e906
Compare
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 |
861e906
to
b95ae02
Compare
tested b95ae02 webdev/servewallet and initChart is called, but somehow the chart isn't loading. ![]() |
actually, I forgot testing after removing the |
b95ae02
to
6f3304b
Compare
(rebase master force push ) |
@thisconnect so it is definitely the public componentDidUpdate(prev: Props) {
const { chartDataDaily, chartDataHourly } = this.props.data;
if (!this.chart) {
this.createChart();
} which translates to the Do you have any env variables for react? Maybe the chart shows for me because react runs |
f065771
to
01f035e
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.
frontends/web/src/api/account.ts
Outdated
@@ -14,8 +14,8 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import { LineData } from 'lightweight-charts'; |
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.
import { LineData } from 'lightweight-charts'; | |
import type { LineData } from 'lightweight-charts'; |
} | ||
initChart(); | ||
return () => { | ||
window.removeEventListener('resize', onResize); |
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.
initChart conditionally adds the event listener, but here it always removes the event.
I think this should be changed somehow. I guess the event can always be added?
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 yes..I wanted to refactor this anyway in another PR but I guess it is a good idea to already do it here. I will add a second commit just for easier review which we can squash before merge.
01f035e
to
fd63af3
Compare
4a2af4b
to
d437512
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.
@thisconnect I addressed all changes in the first commit and added a second one that refactors the chart initialization. When everything is fine I can squash them before merge.
PTAL :)
chart.current?.timeScale().unsubscribeVisibleLogicalRangeChange(calculateChange); | ||
chart.current?.unsubscribeCrosshairMove(handleCrosshair); |
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.
imo. chart.remove()
should clean up everything but from a quick look at the function documentation I don't know if it does so I left the unsubscribing in here just for safety..wdyt @thisconnect should I remove these?
@@ -101,10 +101,11 @@ export const Chart = ({ | |||
const ref = useRef<HTMLDivElement>(null); | |||
const refToolTip = useRef<HTMLSpanElement>(null); | |||
const chart = useRef<IChartApi>(); | |||
const chartInitialized = useRef(false); |
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 guess we could also use chart.current
but I like it like that, it is more clear even if its a bit redundant. but no strong opinion, let me know if I should remove this and use chart.current
(!== undefined) to check if the chart was initialized.
7a8cc75
to
1e241e8
Compare
1e241e8
to
59fa1c2
Compare
d840d99
to
107bb6b
Compare
107bb6b
to
d91a04e
Compare
<header> | ||
<div className={styles.summary}> | ||
<div className={styles.totalValue}> | ||
{formattedChartTotal !== 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.
note: can be undefined
needs to be fixed but wait for master and rebase
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.
did you rebase?
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.
Yes I did rebase, formattedChartTotal
should now be either null
or some value because #2850 now uses success
and error
response.
unrelated to this: I saw some other comments above, I thought I did address all change requests/review but will take another look and ping you if I think it is ready for review again, thanks :)
Convert the Chart class component in chart.tsx to a functional component.
Refactor the charti initialization after the refactor to a functional component so that it only initializes the chart when required and cleans everything up properly when the component unmounts or the chart is removed to re-initialize it. SQUASH in last commit
d91a04e
to
66dce2a
Compare
@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.