visualize-admin / visualization-tool

The tool for visualizing Swiss Open Government Data. Project ownership: Federal Office for the Environment FOEN
https://visualize.admin.ch
BSD 3-Clause "New" or "Revised" License
29 stars 3 forks source link

New chart from different dataset #1527

Closed ptbrowne closed 1 month ago

ptbrowne commented 1 month ago

Ability to add a new chart config based on a new dataset

We can add a new chart config based on a different dataset than the one we currently see.

Looking at it, I am not too happy with how the code is organised for that since I need to drill down on click callbacks, I may have to refactor using a context that would

I'd be happy if you can have a look with this in mind @bprusinowski , if you have an idea ?

vercel[bot] commented 1 month ago

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

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 2 resolved May 28, 2024 0:33am
ptbrowne commented 1 month ago

I cleaned up a bit the part about the two different providers, so that from the consumer side (the page or the drawer), they would only change the variant but would not know about the details of URL sync. I also removed the need for different providers, instead I create the correct hook based on the syncWithUrl parameter. I find it more readable, and the low level of details of using useBrowseStateQueryParams or useState are hidden inside createUseBrowseState.

I will not go further in the de-entanglement for now, thanks for the feedback that apparently props drilling was not too much.

I put it there so to be able to come back to it, but I found myself wanting to be able to control the behavior of Links = have a way to "hook" into the next js Link from the context, and react to it.

const SelectPageDrawer = () => {
   const [dataset, setDataset] = useState()
  return <InterceptableLinkContext onClickLink={(ev, url) => {
     if (url.matches('/dataset')) {
         setDataset(getDatasetFromUrl(url))
         ev.preventDefault()
     }
    }
  }}>
        <SelectPageContent />
   </InterceptableLinkContext>
}

const SelectPageContent = () => {
      return <>
           <Nav />
           <Results />
      </>
}

const Results = () => {
      return results.map(x => {
           return <InterceptableLink href={`/dataset/${x.iri}`}><MuiLink /></InterceptableLink>
       })
}

Is this a good idea I wonder 🤔 .

This way, you would be able to surgically control the behavior of links. You would lose in type safety, whereas with prop drilling, you know exactly what to do, at the expense of verboseness.

bprusinowski commented 1 month ago

Ah, so you'd actually have Links that are Links or Buttons with onClick event, depending on the variant (page vs drawer) – but the components would actually stay the same (in simplification)? I think that's an interesting idea 👍 🥹 And should also be understandable with a small docstring 😅

ptbrowne commented 1 month ago

Yes exactly, because it's good I find to have links with the hover and right click behavior of links inside the page. And also if we can keep the behavior of links for cmd + click and right click > open new tab n the context of drawer, that's good. That's why I want to keep links.