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

Feat/extend supported datasets #1855

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

noahonyejese
Copy link
Contributor

@noahonyejese noahonyejese commented Nov 13, 2024

This PR

Description
Our functions only allowed for DD.MM.YY supported dates on datasets causing issue's w/ the dataset mentioned in #1799 . This PR introduces 10 new supported Time formats increasing supported time formats from 5 to 15. Additionally a lot of components where not formatDate resulting everything to be stacked on top of each other or not visible.


How to test

  1. Since this Dataset is only available on Int or ssl development to Int you can only test this PR once deployed
  2. Go to this link
  3. Select Column-Line Chart See how we have a bunch of NaN.NaN.NaN dates
  4. Select they are gone now view image

image

Copy link

vercel bot commented Nov 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 13, 2024 9:56am

@bprusinowski
Copy link
Collaborator

@noahonyejese dataset selection generally doesn't depend on environment, except we limit data sources to INT and PROD on INT, and to PROD on PROD – every other environment (local, Vercel, TEST) has access to every cube. The one from the issue can be checked with this PR via https://visualization-tool-git-feat-extend-supported-datasets-ixt1.vercel.app/en/browse?previous=%7B%22order%22%3A%22SCORE%22%2C%22search%22%3A%22moderhinke%22%2C%22includeDrafts%22%3Atrue%7D&dataset=https%3A%2F%2Fhealth.ld.admin.ch%2Ffsvo%2Fmoderhinke_entwicklung%2F1&dataSource=Int (you need to enable draft datasets and change data source to INT to find it) :)

Copy link
Collaborator

@bprusinowski bprusinowski left a comment

Choose a reason for hiding this comment

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

Thanks @noahonyejese, good catch with the formatDate thing! 📆

There is one thing I think we need to fix before merging (column charts tooltip, see my comment).

I also wonder if we needed to extend the dates formats, was there a problem with them? If not, maybe we don't need to extend them just for the sake of doing so, I believe they come in a standard format from the Cube Creator 🤔

README.md Outdated Show resolved Hide resolved
app/charts/column/overlay-columns.tsx Show resolved Hide resolved
app/charts/combo/combo-line-column-state.tsx Show resolved Hide resolved
app/charts/combo/combo-line-column.tsx Show resolved Hide resolved
app/charts/combo/combo-line-dual-state.tsx Outdated Show resolved Hide resolved
@noahonyejese
Copy link
Contributor Author

@noahonyejese dataset selection generally doesn't depend on environment, except we limit data sources to INT and PROD on INT, and to PROD on PROD – every other environment (local, Vercel, TEST) has access to every cube. The one from the issue can be checked with this PR via https://visualization-tool-git-feat-extend-supported-datasets-ixt1.vercel.app/en/browse?previous=%7B%22order%22%3A%22SCORE%22%2C%22search%22%3A%22moderhinke%22%2C%22includeDrafts%22%3Atrue%7D&dataset=https%3A%2F%2Fhealth.ld.admin.ch%2Ffsvo%2Fmoderhinke_entwicklung%2F1&dataSource=Int (you need to enable draft datasets and change data source to INT to find it) :)

Ah okay thanks @bprusinowski didn't know that!

@noahonyejese
Copy link
Contributor Author

Thanks @noahonyejese, good catch with the formatDate thing! 📆

There is one thing I think we need to fix before merging (column charts tooltip, see my comment).

I also wonder if we needed to extend the dates formats, was there a problem with them? If not, maybe we don't need to extend them just for the sake of doing so, I believe they come in a standard format from the Cube Creator 🤔

The primary reasons I added multiple while I was at it are:

  • There was a comment saying to extend it by you
  • The Dates where formatted as DD.MM.YY and it was expecting MM.DD.YY causing the initial issue

😅

@bprusinowski
Copy link
Collaborator

@noahonyejese are you sure it was expecting MM.DD.YY? I think the issue was that we mapped formatDate over xDomain, and afterwards didn't format the date (thus the issue with dots and dashes), but I'd be surprised if there was a US-date format that was expected 😱

@noahonyejese
Copy link
Contributor Author

@noahonyejese are you sure it was expecting MM.DD.YY? I think the issue was that we mapped formatDate over xDomain, and afterwards didn't format the date (thus the issue with dots and dashes), but I'd be surprised if there was a US-date format that was expected 😱

@bprusinowski I mean I logged everything and I saw it there. Pick Combo-line Chart here and you'll see some dates are working and some are NaN.NaN.NaN if you look closely you don't even have to use console.log, the last working date is 10.12.2024 and from 10.13.2024 it says NaN.NaN.NaN so I assumed it was having trouble because there is no 13th month, also

It works now (view the picture above)

@bprusinowski
Copy link
Collaborator

@noahonyejese just a small FYI, you can't share /create links, you'd need to publish the chart so I can access it (you can try to go to this link in a private browser) 👀

I hope I'm not too picky, but it looks like it's correct in the data, see a SPARQL query with the full dataset: https://s.zazuko.com/22nfxhH, that's why I doubt that the US-date format can appear here. Did you console.log the observations to see how it's formatted? Maybe we do something on the server side that makes it fail. It's just that I've never seen a problem like that in Visualize, and here also it looks like the date format is correct (EU) on the data side, so maybe it's another bug we need to find?

PS. It seems to work correctly with the current logic in other places (see the image below), that's why I doubt it's a new formatting issue 🤔

Screenshot 2024-11-13 at 11 59 43

@bprusinowski
Copy link
Collaborator

I think I'd try to see why column chart X axis works correctly (without new date formats), and then see what's different in combo charts, to narrow down the issue 👀

@noahonyejese
Copy link
Contributor Author

@bprusinowski Thanks for your insights, maybe that's true I will try to compare it. Maybe it's another issue, but that's still quite a weird because I console.log(x) the output was 10.10.2024, 10.11.2024

etc

Then I console.logged xScaled which was undefined

 const x = getX(d);
    const xScaled =
      (xScale(formatDate(x)) as number) + xScale.bandwidth() * 0.5;

also xScaled was only undefined for values above 10.12.2024 I will further investigate this tho.

@bprusinowski
Copy link
Collaborator

bprusinowski commented Nov 13, 2024

Thanks @noahonyejese, yeah, but 10.10.2024 and 10.11.2024 are still valid dates with our current formatting, right? I think it would be super weird if this bug was related to US-date formatting, but only for combo charts, that's why I was a bit pushing against adding a new date format options 😅

I think with the comment to add more formats, I was thinking about more formats like "decade", "microsecond", etc, but not about introducing MM.DD.YYYY ones, unless we really see that the data has it defined in such way.

@noahonyejese
Copy link
Contributor Author

Thanks @noahonyejese, yeah, but 10.10.2024 and 10.11.2024 are still valid dates with our current formatting, right? I think it would be super weird if this bug was related to US-date formatting, but only for combo charts, that's why I was a bit pushing against adding a new date format options 😅

I think with the comment to add more formats, I was thinking about more formats like "decade", "microsecond", etc, but not about introducing MM.DD.YYYY ones, unless we really see that the data has it defined in such way.

I see, I will investigate this

@noahonyejese noahonyejese self-assigned this Nov 20, 2024
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.

Inconsistent horizontal axis values in Column-Line Chart type
2 participants