Chartjs attempt #13897
Chartjs attempt #13897
Conversation
* master: (254 commits) fix team update should not overwrite all fields - closes lichess-org#13869 simul host.provisional is optional remove players in wrong variant on update scalafmtAll refactor show host proper rating with provisional Lpv scala code golf New Crowdin updates (lichess-org#13911) move to ctx.canPalantir ctx.kid: KidMode use ctx.kid in Account.info scalachess 15.6.9 MsgSearch needs to know if the current request is in kid mode read canPalantir from ctx, not from user allow forcing kid mode with http header (lichess-org#13882) update MarkdownTest scalafmt distinguish links to private studies preventing moves list items from flickering and or dissapearing in explorer do not allow embedding private studies ...
* master: crowdv now only in schlawg branch correct the commit that fails to download in our unused, broken script open relation API endpoints to the mobile token update 2FA message to include titled players scalafmt Update play-json to 3.0.1 Do not use `xhr.json` on study endpoints returning no content
|
Looking promising. We don't need to port all the charts right away. I'd prefer to see one or two charts being completed, which we can merge. Then we attack the next ones later on. What's missing to ship, say, the ACPL and movetime charts? |
… into chartjs-attempt
|
Functionally, it should all work. I'm mostly not happy with my code quality. There are lots of improvements to be made. Problems:
Wins:
It was just a note to self to add dynamic theme-changing which was added with 83b866d along with other misc changes. Insights support added with 850a98f |
|
Thanks.
I did notice an increase in the minified size, yes. Because the charts are not on critical pages (like homepage and gameplay), and because we can render the full page layout before loading them, I think it's ok.
As long as we only load one chartjs instance per page, and only after (rest of) the page is rendered and functional, then that's ok by me.
A bit sad as I think it was nice to see directly on the chart. Kinda incredible that it adds 30kb minified JS. What even is in chartjs? Of course there will be bugs, but that's ok, we'll sort them out. |
|
I think the ACPL and movetime charts already look good enough. Could we have bigger and/or more visible hover points, and also while litting up the "christmas tree"? |
|
Is it possible to retain the horizontal animation from https://lichess.org/stat/rating/distribution/blitz? I think the default chartjs vertical animation doesn't look as good on that page. |
* master: (27 commits) replace with space in french translations remove copy-pasted code that's also unnecessary wrapping an int in NumberLong is without effect and triggers a warning scala tweak remove unused opaque type add snabbdom key to notification to prevent DOM reuse - fixes lichess-org#13645 remove unused python os import New Crowdin updates (lichess-org#13924) remove dubious and duplicated quotes scala tweaks Allow different asset url for requests from Dasher prettier fix games page overflow fix safari autoplay text ignore games over 4 hours hopefully touch drags enable audio context on ios safari now Fix translations Fix formatting Add translations remove confusing, unnecessary indirection ...
… into chartjs-attempt * 'chartjs-attempt' of https://github.com/allanjoseph98/lila: Don't animate the christmas tree Animate charts ltr on load. +misc. fixes
|
Almost there! Looks like ui/insight/chart could depend on chart/common to reuse the grid/label colors. |
|
This removes some godawful hacks I was hoping to sweep under the rug one day. Soon I will be able to pretend I never committed them! |
|
I don't want to throw a complication in here last minute, but Allan if your efforts were constrained because you were worried about that extra 30k... I assume there's a reason chart.js + annotations plugin can't be loaded (and cached browserside) dynamically as a module? |
|
I haven't looked into it yet but it should be possible in theory. I prioritised replicating the charts first and then looking at optimisations later so I never got around to looking into it. Compared to highcharts, a similar Chartjs + annotations implementation would be an extra ~30+30=60kb. Essentially, the only thing missing is the text next to the annotation line. There is also the movetime chart size issue as a result of hacks that I highlighted earlier. I didn't think that displaying extra text was worth a 30kb additional download so I never considered the plugin again. But yeah, you make a good point that since it would be cached it might not be too bad. I leave it up to you guys to decide. |
|
Ok. Down the road, we can look into a custom bundle module with only the things we need from chartjs. Feel free to leave comments about any imports or code that bloat concerns stopped you from adding. Maybe it can be added when/if the module comes along. |
* master: (31 commits) Revert "new PGN export patron tags, optional" New Crowdin updates (lichess-org#13949) pnpm format open POST /account/profile to mobile app dedup anon challenges scala tweaks require a sessionId to make an anon challenge fix racer zen alignment and content Update netty-transport-native-epoll to 4.1.101.Final Copy unmovedRooks when creating rematch games new PGN export patron tags, optional Fix removing fields from teams New Crowdin updates (lichess-org#13946) titles don't need to end with ":" too many newlines use automatic <br> with HTML translations don't manually edit I18nKeys, generate them with ./bin/trans-dump rename CC button - closes lichess-org#13945 don't expand images in streamer mod notes fix new lines in translated HTML ...
Looks like all the old annotations now.
|
This is great, thank you so much. I'm merging this right now and will deploy it in the days to come, so we can get user feedback. |
WIP. Needs a lot of cleaning. I've tried to keep the look and function as similar to the current one as possible.