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

Visualise changes over time #175

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Visualise changes over time #175

wants to merge 32 commits into from

Conversation

@codemacabre codemacabre self-assigned this Nov 28, 2024
@codemacabre codemacabre marked this pull request as ready for review November 28, 2024 14:12
src/utils/bods.js Outdated Show resolved Hide resolved
@radix0000
Copy link

Okay, had to hand roll my own BODS 0.3 test data (because examples are so broken):

https://gist.github.com/radix0000/5e612d582b41a8aca5bfff4d5e396bf8

So doesn't show change over time with BODS 0.3 (which is fine if that is the intention), but does go into "change-over-time mode" with slider displayed if there is a replacesStatement property present (you just can't slide the slider, and all statements displayed), but latest time it shows conflicts with the fact all statements are shown, which is a bit confusing. So if it is intended that doesn't support change-over-time for 0.1-0.3 then recon that slider should always be hidden regardless of whether there is a replacesStatement property or not.

@codemacabre
Copy link
Collaborator Author

codemacabre commented Dec 13, 2024

@radix0000 Thank you. @kd-ods and I discussed this at length and came to the decision that 0.4 data renders and interactive slider, whereas pre-0.4 data with a change-over-time element renders an inactive slider to indicate that there is change-over-time data, but not render the changes themselves. The slider is hidden only if there is no change-over-time data detected.

but latest time it shows conflicts with the fact all statements are shown

I think this is an issue with the data. If you use this data, it works as expected. If you use your data in the current (old) visualiser, it renders much the same as here.

codemacabre added a commit that referenced this pull request Dec 13, 2024
@radix0000 radix0000 self-requested a review December 13, 2024 14:23
Copy link

@radix0000 radix0000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One thing I happened to notice when testing is that data with the wrong version (i.e. bodsVersion == "0.4" when it is actually 0.3 format) results in a "Your data does not have any information that can be drawn." message, and while it should certainly give an error, ideally it could be more informative (especially as the only difference between data that works and doesn't, is one character). Possibly having a check for "recordDetails" property and whether its presence or not matches the version, < 0.4 vs >= 0.4, could work. I am doing something similar in the data review tool). But don't think it is a show stopper, so happy for this to be merged either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment