feat(charts-ng): improve charts accessibility#1735
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces default ARIA properties to the chart options if they are not already defined. The review suggests refactoring this change to avoid direct mutation of the options parameter, promoting pure transformations and preventing potential side effects, which aligns with the repository's style guide.
| if (!options.aria) { | ||
| options.aria = { | ||
| enabled: true, | ||
| label: { | ||
| enabled: true | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
To avoid side effects, it's a best practice to not mutate function parameters. Modifying the options object directly can lead to unexpected behavior in other parts of the application that might be using the same object.
Instead, I suggest creating a new object by spreading the original options and adding the default aria properties. This ensures the function does not have unintended side effects and aligns with the principle of pure transformations.
if (!options.aria) {
options = {
...options,
aria: {
enabled: true,
label: {
enabled: true,
},
},
};
}References
- The repository style guide recommends keeping state transformations pure and predictable (line 36). Although this rule is under the 'State Management' section, the principle of avoiding side effects by not mutating inputs is a general software engineering best practice that applies here. Mutating function arguments is an impure transformation that can cause unpredictable behavior. (link)
a2edff8 to
7d9debe
Compare
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: