Skip to content
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

Small fixes #1

Merged
merged 1 commit into from May 20, 2022
Merged

Small fixes #1

merged 1 commit into from May 20, 2022

Conversation

phocks
Copy link
Member

@phocks phocks commented May 20, 2022

Hi @jtfell the charts all look good and the functions for the scatterplots seem ok. I'd just suggest maybe trying to align the tooltip when hovering to the right of the chart so not to overspill, but it's not too important, especially if we'll be displaying on pages with a right sidebar anyway.

Also I noticed the top and bottom dots are always right up the top and right down the bottom due to the extent clipping and they go above and below the axis labels. Again, maybe ask Ben if that's acceptable. We can kind of add margins by changing the .range values, but then it screws up the y axis line a bit.

$: yScale = scaleLinear()
    .domain(extent(data, (d) => d.y))
    .range([innerHeight - 30, 0 + 30]);

Anyway It all seems pretty good, but yeah as I mentioned the errors (if any) will probably come out when testing different types of charts with the data.

Feel free to merge or just copy this change. It's pretty much just fixing a VS Code TypeScript error I was getting.

If anything major breaks down the track, I can give you a hand as I've got kinda my head around the code now.

elections-federal2022-scatterplots

elections-federal2022-scatterplots2

@phocks phocks requested a review from jtfell May 20, 2022 04:50
@jtfell
Copy link
Collaborator

jtfell commented May 20, 2022

Thanks @phocks! I'll talk to Ben about the margins/tooltips. I think he's keen to make maximal use of screen real estate on mobile, so having dots on the axes is probably fine. Will confirm though.

And yeah, good to have you across it. Never know what will happen on the night!

@jtfell jtfell merged commit e96b7dc into main May 20, 2022
@jtfell jtfell deleted the josh-fixes branch May 31, 2022 00:39
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.

None yet

2 participants