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

style: Update metadata and filters positions + styles #1552

Merged
merged 10 commits into from
Jun 3, 2024

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Jun 3, 2024

Closes #1528

How to test

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

@bprusinowski bprusinowski requested a review from ptbrowne as a code owner June 3, 2024 11:26
Copy link

vercel bot commented Jun 3, 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 Jun 3, 2024 3:11pm

@ptbrowne
Copy link
Collaborator

ptbrowne commented Jun 3, 2024

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
image

@bprusinowski bprusinowski force-pushed the style/metadata-button branch from 8a2d81e to 7d8e288 Compare June 3, 2024 14:34
@bprusinowski
Copy link
Collaborator Author

@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 👍

Copy link
Collaborator

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I think it looks better already.

LGTM at the current stage, no need to go further. I had thought of the zustand store because I thought it could also keep the state of whether the metadata panel was opened or not. But it could be easily done later. The interactive filters could also go there. Actually the current interactive filters stores could maybe be extended with the filters state and the metadata panel state.

Let me know what you think, but I do not think it should be done in this PR :)

@bprusinowski
Copy link
Collaborator Author

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 :)

@bprusinowski bprusinowski merged commit 319fde6 into main Jun 3, 2024
5 of 6 checks passed
@bprusinowski bprusinowski deleted the style/metadata-button branch June 3, 2024 15:13
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.

✅ UI improvement: 'Metadata'- button and 'show filter'- button aligned underneath chart title
2 participants