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

fix: Minimum dashboard chart sizes #1734

Merged
merged 11 commits into from
Sep 10, 2024
Merged

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Sep 9, 2024

Fixes #1732

This PR:

  • fixes some issues with tab labels in layouting step,
  • fixes computations of minimal height of free canvas when adding a new chart to an existing layout,
  • increases minimal height of free canvas chart from 100 to 150 pixels, which should help with wrong initializations in some cases,
  • improves Table appearance in free canvas layout (correct initial sizing and non-overflowing content).

There's still a potential to improve the table chart in free canvas further, as it looks that compact view doesn't work as expected in small sizes and scrollbar position doesn't seem right, but this can be done in another PR, as it seems like a table issue, and not free canvas-related problem.

How to test

  1. Go to this link.
  2. Add a new table chart.
  3. ✅ See that the active tab's chart icon is blue, and inactive icon is black.
  4. Proceed to layout options.
  5. ✅ See that the Label tab is not available under the Title & Description section in the sidebar.
  6. ✅ See that the chart icons are visible in the tabs.
  7. Switch to a dashboard / free canvas layout.
  8. ✅ See that the charts were resized correctly.
  9. Go back to the editing mode.
  10. Add a new, pie chart.
  11. Proceed to layout options.
  12. ✅ See that the pie chart was correctly initialized.

How to reproduce

  1. Go to this link.
  2. Add a new table chart.
  3. ❌ See that the tabs are both gray.
  4. Proceed to layout options.
  5. ❌ See that the Label tab is available under the Title & Description section in the sidebar.
  6. ❌ See that the chart icons are missing from the tabs.
  7. Switch to a dashboard / free canvas layout.
  8. See that the charts were not resized correctly (e.g. table is not visible at all).
  9. Go back to the editing mode.
  10. Add a new, pie chart.
  11. Proceed to layout options.
  12. ❌ See that the pie chart was not correctly initialized.

Copy link

vercel bot commented Sep 9, 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 Sep 10, 2024 1:18pm

Copy link
Contributor

@noahonyejese noahonyejese left a comment

Choose a reason for hiding this comment

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

You are right about the free Canvas view there is still a bit of room for improvement.

@noahonyejese
Copy link
Contributor

Nit: Quite a lot commits, you could potentially split them up a bit? Other than that it looks fine!

@bprusinowski bprusinowski force-pushed the fix/minimum-dashboard-chart-sizes branch from 17342a5 to 763f68e Compare September 10, 2024 12:27
@bprusinowski
Copy link
Collaborator Author

Thanks for the review @noahonyejese! You're right, I should have cleaned up the commits, as there was some exploration that didn't end up being used. I reorganized and merged the commits, so it went down from 15 to 10 👍

@noahonyejese
Copy link
Contributor

👍🏽 not too bad

@KerstinFaye
Copy link

thanks a lot @bprusinowski. It so much improved already. that's fantastic.
I saw something very small. Let's call the section under title & description "Tab label", instead "Label". I think this would be a bit more clear. (I could not identify the correct position to change this in Accent).

Another thing that I discovered in the free canvas step is, when increasing one of the charts (tested it with the table chart), and then clicking through the preview buttons, and after getting the sidebar to edit again, the chart is always jumping back to the minimum height:

Screen.Recording.2024-09-10.at.14.55.48.mov

@bprusinowski
Copy link
Collaborator Author

bprusinowski commented Sep 10, 2024

Hi @KerstinFaye, thanks for testing and feedback, I'll change the label 👍

For the issue with going back to minimum height, in fact it goes back to original height set before entering the preview. I think we could either disable an ability to move the charts around and resize them while in preview mode (quick thing to do, maybe good anyway to not confuse the users?) or make it possible to edit the layouts when in preview mode, currently it's not yet implemented :/

Let me know what would make more sense from your side, I believe it could be nice to be able to edit while in preview mode to be able to e.g. optimize the mobile layout. But I'd do this in another PR 👀

@KerstinFaye
Copy link

@bprusinowski yes, I really like the ability to change the layout while in preview mode. But would it be possible to save the height and width that was set for the charts when changing between the preview buttons? So when going back to the maximum size the user sees the height that they set before?

@bprusinowski
Copy link
Collaborator Author

bprusinowski commented Sep 10, 2024

@KerstinFaye I think we could keep different layout configurations at all times, currently we only keep lg when on a big screen, and smaller ones are generated and removed "on the fly", as needed, that's why we lose the settings. But it means that we would actually edit three different layouts, so a change made while on smallest one, would only be applied to it and not be carried over to largest one.

In this case I'll focus on improving this in a next PR, we could also discuss this tomorrow if the behavior is not clear 👍

@bprusinowski bprusinowski merged commit 0e495e8 into main Sep 10, 2024
5 of 6 checks passed
@bprusinowski bprusinowski deleted the fix/minimum-dashboard-chart-sizes branch September 10, 2024 13:45
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.

✅ Improve minimum size of charts in dashboard
3 participants