vmware-archive/clarity

clrDgSelected on DatagridRow behaves inconsistently

juanmendes opened this issue · 5 comments

juanmendes commented

Describe the bug

When upgrading to Clarity 12. I noticed that the @Input('clrDgSelected') of ClrDatagridRow which is bound to the selected getter and setter now take a string | boolean. I was told by @bdryanovski that this was to allow fullTemplateTypeCheck to pass when you use text attributes.

However, when this was implemented, the only change that happened was that the type of the setter and getters was changed to be string | boolean but it doesn't actually handle the case where it's a string. It just blindly casts any string that may come in as a boolean.

If a component is going to accept a bare boolean attribute as a string, it should behave like HTML's boolean attributes, that is, it should be determined by the presence of the boolean attribute, not its value.

 <input disabled /> <!-- Input is disabled --!>
 <input disabled="false" /> <!-- Input is disabled --!>
 <input disabled="true" /> <!-- Input is disabled --!>
 <input [disabled]="false" /> <!-- Input is enabled --!>
 <input [disabled]="true" /> <!-- Input is disabled --!>

However, since Clarity is not doing anything to properly parse the string/boolean attribute, it has weird behavior that will be described in the "How to Reproduce" Section

How to reproduce

See an example stackblitz which shows multiple ways of setting clrDgSelected and its incorrect behavior

image

Steps to reproduce the behavior:

  1. Go to https://stackblitz.com/edit/clarity-light-theme-clr12-cds5-hxypjp?file=src%2Fapp%2Fapp.component.ts
  2. Notice each test case on the grid, it shows the binding used and its result (whether the checkbox is checked)

Expected behavior

I expected the following bindings to make rows selected (if they are supposed to follow the boolean HTML attribute standard, as in <input disabled />), but they are not selected:

clrDgSelected
clrDgSelected=""
clrDgSelected="true"
clrDgSelected="false"

Clarity project:

  • Clarity Angular/UI

Clarity version:
Clarity 12

Framework:

  • Angular

Framework version:
Angular 12

Device:
Any

Additional notes

The behavior is inconsistent and seems to be undefined. If the current behavior is expected, it should be documented with an explanation as to why.

Here are different attempts to properly implement boolean text attributes:

coryrylan commented

Thank you for the detailed stackblitz! Yes we should be handling the runtime coercion for the boolean strings here. We also need to update the type signature to better represent the setter values. We can follow similar value type in material as seen here
https://github.com/angular/components/blob/master/src/cdk/coercion/boolean-property.ts

Additional details:
https://angular.io/guide/template-typecheck#input-setter-coercion

juanmendes commented

Thanks for the quick response. If possible, can we also have access to the coercion routine? I have a lot of test code that is wrapping the selected property with the following and I don't want to propagate string | boolean to its callers, I'd like to coerce it all from the same place if possible.

    isFirstRowSelected(): boolean {
        return !!this.component.entitySelector.clarityGrid.rows.first.selected;
    }

The !! is the wrong solution here and I'd like to use the same solution that you end up using. Thanks

juanmendes commented

Sorry, one more note. It feels like the change to allow bare boolean attributes for clrDgSelected seems unnecessary. I have never had to use it and it feels like it would only be useful when hardcoding a datagrid for demonstration purposes, as I did in my demo of the error. But since it's already in there, may as well just touch up its behavior.

ashleyryan commented

Hello, this repository has been archived. If your issue still exists, please recreate the issue in one of the new repositories:

github-actions commented

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.