search

vikramlearning / blazorbootstrap

An Enterprise-class Blazor Bootstrap Component library built on the Blazor and Bootstrap CSS frameworks.
https://docs.blazorbootstrap.com/
Apache License 2.0
565 stars 15 forks source link

Added Chart options, plus some QOL improvements. #737

Closed Lucasharskamp closed 3 weeks ago

Lucasharskamp commented 1 month ago
  1. Charts severely lacked styling options that the Chart.JS library provides. I've added a number of options that I needed for my charts for my employer's code. This includes new options for:
  1. In Blazorbootstrap user data input is often demanded to be in the form of List<T>; this is wrong, because the code can work with any collapsed stack since the data is only read, not modified. This puts IEnumerable<T> out of the question (because of multiple iteration issues that can go haywire), but IReadOnlyCollection<T> gives the programmer more options to work with, e.g. providing an array, List<T>, ImmutableList<T>, ArraySegment<T>, FrozenSet<T>, HashSet<T>, SortedSet<T>, Stack<T>, etc etc etc. So at the very least, this prevents the user from applying a redundant ToList() that only steals performance/memory anyways. Blazor Bootstrap doesn't modify the list anyways, only reads it. And whereas IEnumerable<T> is easily out of control, this happens far less with IReadOnlyCollection<T> and would require dedication on the part of the programmer to get that to happen.
  2. Documentation uses <see cref="https..." />, but it must use href=
  3. Nullable properties that will not be serialized if null, should not be auto-initialized.
  4. <see langword="..." /> helps provide proper markup for code. As an example, I've replaced "if true, then" with the langword on property summaries.
  5. One must not throw NullArgumentExceptions if the null issue is within a parameter,
  6. Generate a nuget package on build in debug/release folder. Very useful for testing purposes.
  7. Bugfix: if a chart is larger than 300px/150px, then the width/height settings are ignored and instead the chart sizes itself to 300px/150px regardless of wrapper size. By setting the width/height in the properties, this resolves that issue. I have tested this for responsive charts and seen no issues (so far)

(These QOL improvements in the summary documentations are very useful for AI coding tools like Copilot, since that gives the AI better directions when generating code for the user)

Notes about Blazorboostrap I want to tackle later

  1. Blazor Bootstrap ChartJS still lacks a lot of the features of ChartJS, I might add those later, this is just a first iteration.
  2. ChartJS: background color/image plugin
  3. Figuring out a way to generate JS methods for certain ChartJS function callbacks (e.g. what color to give to a gridline based on a value)
  4. Settings classes should use a parameterless constructor and a constructor (with default values/nulls) that set all the properties up, to prevent the need for verbose coding on implementation. Perhaps making them Records is better? They're readonly anyways.
  5. Many classes and properties in the library still lack in-code documentation and code examples. This isn't just for the human reader, but like mentioned the AI tools.
  6. Loads of redundant null checks for items that we know are in a valid state, or properties/parameters lacking a nullability operator in their contract.
  7. Many methods aren't marked static despite not altering the object, causing needless objects to be created/consumed.
  8. Tons of improper Capitalisation on properties and methods. And no, JS Invoked methods do not need to start with a lowercase.
  9. Checking for item existence in collections prior to removal/adding/updating isn't required anymore, we're not on Framework.
  10. Enable .NET 7
  11. Number/currency input for .NET7/8 can use INumber<TSelf> using macros in code to separate it from .NET 6 usage. Also allows one to use C#11 for .NET 7/8.
  12. Certain properties cause a StateHasChanged invocation, which is fine for .NET 6 but not for 7/8. Macros will fix this.
  13. See https://github.com/vikramlearning/blazorbootstrap/issues/711
gvreddy04 commented 1 month ago

@Lucasharskamp Thank you for the PR and for your time. I'll review this tomorrow.

Lucasharskamp commented 1 month ago

@Lucasharskamp Thank you for the PR and for your time. I'll review this tomorrow.

Alright, I'll look forward to hear from you

gvreddy04 commented 4 weeks ago

@Lucasharskamp Thank you very much for your detailed feedback, PR (including XML comments) and future contributions details.

Lucasharskamp commented 4 weeks ago

Can you provide a code sample, screenshot, or recording for better understanding?

In the original, the following code:

image

creates this:

image

instead of this:

image

These examples will be helpful to others.

No need. This isn't something that affects features, it's a simple bugfix for spacing issues.

I've got another PR ready to go after this one which overhauls a lot of coding issues

(methods that should be static, tasks not passed properly through the callstack, nullability issues, loads of added XML commenting, requested datatypes always going to IReadOnlyCollection<T> if it's read-only anyways, etc) These changes do not alter the way users use the library, with one exception: IEnumerable<T> isn't allowed anymore for data collection input to prevent multiple-iteration-issues, but those are easily rectifiedable` isn't allowed anymore for data collection input to prevent multiple-iteration-issues, but those are easily rectified. https://stackoverflow.com/a/51484883 https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.ireadonlycollection-1

Lucasharskamp commented 3 weeks ago

@gvreddy04 can you please handle my 2 PR's? I got more coming up, namely a Bootstrap css generator (so we don't have to suffer the indignities of node.js anymore with scss or creating manual overrides for colors and styling)