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

Chart size updates #384

Merged
merged 12 commits into from Nov 10, 2023

Conversation

xramcire
Copy link

https://www.chartjs.org/docs/latest/configuration/responsive.html
Following chart.js specs:
Allow chart container sizes to be set via width and height attributes.
Allow chart container sizes to be set to any unit of measure (%, v[h|w], px, etc).
Allow chart size to fill it's container.
Allow disabling of MaintainAspectRatio.

Following chart.js specs:
Allow chart container sizes to be set via width and height attributes.
Allow chart container sizes to be set to any unit of measure (%, v[h|w], px, etc).
Allow chart size to fill it's container.
Allow disabling of MaintainAspectRatio.
Following chart.js specs:
Allow chart container sizes to be set via width and height attributes.
Allow chart container sizes to be set to any unit of measure (%, v[h|w], px, etc).
Allow chart size to fill it's container.
Allow disabling of MaintainAspectRatio.
@gvreddy04
Copy link
Contributor

gvreddy04 commented Sep 30, 2023

@xramcire Thank you for your pull request. When I was testing it, I found something weird. Can you please review it once ? Please see the attached video for your reference.

Start up project: BlazorBootstrap.Demo.WebAssembly

blazorbootstrap_pr_384.mp4

@xramcire
Copy link
Author

Will do!

MaintainAspectRatio is supposed to be true by default. If it's set to false valid height and width must be supplied.
@xramcire
Copy link
Author

I misread the docs from chart.js. The MaintainAspectRatio property needed to be true by default. This is now set.
As far as the values for Width and Height. I added checks to set the unit of measure to "px" so the current demos and anyone else's charts should render correctly. I'm not thrilled at having to inspect the strings but it works. If someone has code that sets the Width or Height programmatically this would be a breaking change. With that in mind I would understand if you want to close this PR.

This will compile: <BarChart @ref="barChart" Width="200" Class="mb-4" />
This will not: barChart.Width = 200;

The .NetFramework has the concept of a Unit. You or I could introduce something along those lines. That would also be a breaking change.
https://learn.microsoft.com/en-us/dotnet/api/system.web.ui.webcontrols.unit?view=netframework-4.8.1

@xramcire
Copy link
Author

@gvreddy04 Any update in this?

@gvreddy04
Copy link
Contributor

@xramcire Sorry for the late response. I prefer Unit Parameter instead of manually typing the unit types. Added Unit enum in the v1.10.3 release. Can you use the Unit enum parameter? Also update ResizeAsync(float width, float height) to ResizeAsync(int width, int height).

We can use something like $"width:{Width.ToString(CultureInfo.InvariantCulture)}{Unit}" and $"height:{Height.ToString(CultureInfo.InvariantCulture)}{Unit}".

@xramcire
Copy link
Author

Sounds good. I'll make the requested changes.

@gvreddy04
Copy link
Contributor

@xramcire Thank you for making the requested changes. One question, do you think user will use two different units for width and height. Can we use Unit param instead of WidthUnit and HeightUnit. All other changes good to me. I'll test this PR later and add if any demo scenarios required.

@xramcire
Copy link
Author

@xramcire Thank you for making the requested changes. One question, do you think user will use two different units for width and height. Can we use Unit param instead of WidthUnit and HeightUnit. All other changes good to me. I'll test this PR later and add if any demo scenarios required.

My use case has a chart of fixed height and varable width. However my use case is less important than the fact that CSS and ChartJS both support arbitrary units and unit types for height and width. We can never know every use case.

I must admit I find the two properties less attractive and less intuitive than the more traditional value. This is likely why Microsoft uses the Unit struct. As we discussed earlier this would be a breaking change for you but I think in the end would make for a more intuitive product.

In markup you could support a struct without it being a breaking change:
Width="90" (defaults to px)
Width="90px"
Width="100%"

In code you would still have a breaking change in:
Chart.Width = new Unit(90); (defaults to px)
Chart.Width = new Unit("90px");
Chart.Width = new Unit(90, UnitType.Px);
Chart.Width = new Unit(100, UnitType.Percentage);
Chart.Width = new Unit(10.5, UnitType.Rem);

Unit => https://learn.microsoft.com/en-us/dotnet/api/system.web.ui.webcontrols.unit?view=netframework-4.8.1
UnitType => https://learn.microsoft.com/en-us/dotnet/api/system.web.ui.webcontrols.unittype?view=netframework-4.8.1

On a slightly related note. CSS and ChartJS both support doubles for width and height. My use case does not require this but someone elses might in the future. Doubles are far more common when declaring sizes in EM and REM.

Either way, let me know if I can help.

@gvreddy04
Copy link
Contributor

gvreddy04 commented Nov 4, 2023

@xramcire Thanks for the updates. I'm testing this pull request. Please watch out for any notifications.

@gvreddy04 gvreddy04 changed the title Features/chart size updates Chart size updates Nov 5, 2023
@gvreddy04 gvreddy04 added area-charts enhancement New feature or request labels Nov 5, 2023
@gvreddy04 gvreddy04 added this to the 1.10.4 milestone Nov 5, 2023
@gvreddy04 gvreddy04 merged commit 56200d9 into vikramlearning:master Nov 10, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-charts enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants