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

Added Chart options, plus some QOL improvements. #737

Merged

Conversation

Lucasharskamp
Copy link
Contributor

@Lucasharskamp Lucasharskamp commented May 29, 2024

  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:
  • axes -> border
  • axes -> grid
  • axes -> title
  • axes -> ticks (-> major)
  • plugins -> legend
  • plugins -> legend -> label
  • plugins -> legend -> title
  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 Ribbon ''Class'' appears on the wrong place #711

Set collection properties for chart settings to IReadOnly to allow programmers to enter any valid stack.
Added options for Charts to allow for more styling options build into ChartJs
@gvreddy04
Copy link
Contributor

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

@Lucasharskamp
Copy link
Contributor Author

@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
Copy link
Contributor

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

  • 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) - Can you provide a code sample, screenshot, or recording for better understanding?
  • Please add a few examples to the following directory: https://github.com/vikramlearning/blazorbootstrap/tree/main/BlazorBootstrap.Demo.RCL/Components/Pages/Charts.
    These examples will be helpful to others.

@Lucasharskamp
Copy link
Contributor Author

Lucasharskamp commented Jun 2, 2024

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 Lucasharskamp mentioned this pull request Jun 3, 2024
@Lucasharskamp
Copy link
Contributor Author

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

@gvreddy04 gvreddy04 added this to the 2.3.0 milestone Jun 7, 2024
Copy link
Contributor

@gvreddy04 gvreddy04 left a comment

Choose a reason for hiding this comment

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

LGTM

@gvreddy04 gvreddy04 merged commit f17cda7 into vikramlearning:main Jun 7, 2024
2 of 3 checks passed
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.

None yet

2 participants