style: Update metadata and filters positions + styles

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

style: Update metadata and filters positions + styles #1552

Closed bprusinowski closed 1 month ago

bprusinowski commented 1 month ago

Closes #1528

How to test

Go to this link.
Select any dataset.
Start making a chart.
See that the metadata button and show filters buttons are in the same row, after chart's title.

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	💬 Add feedback	Jun 3, 2024 3:11pm

ptbrowne commented 1 month ago

I think the button to display the filters and the filters themselves should be decoupled. I find the position: absolute solution too fragile, and the it ties together too much the components (one has to know that another one is in position: relaive, and a height needs to be put for the container).

I think we see here that the buttons to toggle and the display of what they toggle is too intertwined.

I think there should be a zustand store per chart where filtersShown and metadataPanelShown could be stored
Then the MetadataPanel can be split into the button and the drawer
Same for the filters (I can also see that we need heavy use of sx on the toggle filters button, not from this PR through, but the styles seem unnecessary)
Then it is easy to compose both buttons

bprusinowski commented 1 month ago

@ptbrowne I refactored a bit to separate ChartDataFiltersToggle and ChartDataFiltersList; it looks that they two share a lot of common state that probably makes more sense in useState that zustand store, where I'd expect more high-level things like open, setOpen, and not things needed to render the inners of the component, especially if we share one store for two components, like e.g. "useChartControlsStore".

Let me know if it looks better now; otherwise I can move it one step further and actually refactor into a common zustand store 👍

bprusinowski commented 1 month ago

I think it would make sense, but then it should probably be renamed to something like "chartControlsStore" that would keep the state of both interactive filters, data filters element and metadata panel? All in a way "control" the chart 😅

I also wonder if we should simply keep state of UI elements there for data filters and metadata panel (open, setOpen), leaving the actual state close to the components?

I think this could be a part of technical improvements in the next sprint :)