Stringifying boolean and defaulting undefined on aria-busy value on Button Component

[tareqlol]

Component to amend

Button

Visual

only html changes, nothing will affect the visual.

Context

the aim of this fix is to solve two issues:

• Providing undefined to isLoading will skip adding aria-busy property.
• Providing a boolean to isLoading will result aria-busy={false} instead of aria-busy="false" in the compiled html. this is unneeded since this comment

[robinmetral]

Providing undefined to isLoading will skip adding aria-busy property.

I think it should actually pass aria-busy="false", so we probably want to make this a stringified Boolean (just like this)

[tareqlol]

I see, I have a suggestion of making value to be explicit to string like this:

        {...(loadingLabel && {
          'aria-live': 'polite',
          'aria-busy': isLoading ? 'true' : 'false',
        })}

[robinmetral]

This would work but I'd suggest changing the condition for adding these attributes to the element (currently a truthy check for loadingLabel): both aria attributes should only be added if the button can load, i.e. if it has isLoading = true | false.

So maybe we can replace your suggestion by:

-        {...(loadingLabel && {
+        {...(loadingLabel && typeof isLoading === boolean && {
          'aria-live': 'polite',
-          'aria-busy': isLoading ? 'true' : 'false',
+          'aria-busy': isLoading.toString(), // by this point we know `isLoading` is a boolean
        })}

Hope I'm not missing something but I think this would work 🙂

[tareqlol]

true, I will amend that. thanks!

[tareqlol]

@robinmetral I have tried the .toString() first before doing my suggestion, it will cause typescript to complain that the you can not assign aira-busy to string | boolean | undefined to Booleanish | undefined so by explicitly assigning it to isLoading ? 'true' : 'false' solves it with no complain.

what do you think, should I go with my change instead, or do you have an idea how to workaround this 🤔 ?

[robinmetral]

isLoading ? 'true' : 'false' should be fine as well 🙂

[robinmetral]

TIL: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/33d69f6f3cd921193a067df630a7556f04305016/types/react/index.d.ts#L58

Booleanish is Boolean | 'true' | 'false', which means that aria-busy is typed by React to accept stringified or non-stringified booleans. So you could also just pass aria-busy: isLoading, and it should work 🙂

[tareqlol]

updated