Bug: Refactoring host element inconsistencies

[driskull]

Actual Behavior

Some of the components are setting attributes on the host element and they maybe shouldn't be. Lets discuss what to do in these scenarios.

Expected Behavior

Host element should only have needed props and attributes applied to it. Things like aria roles, tabIndex, id and event listeners.

Reproduction Steps or Sample

1. accordion-item: <Host aria-expanded={this.active.toString()} icon-position={this.iconPosition} tabindex="0"> Why iconPosition? Should it be a prop? iconPosition is a private variable but gets set on the host element.
2. alert: <Host active={active} aria-hidden={hidden.toString()} aria-label={this.label} calcite-hydrated-hidden={hidden} queued={this.queued} role={role}> Why queued and active?
3. color-picker-swatch: <Host aria-label={hex} title={hex}> Why title? Should it be internal?
4. combobox-item: <Host aria-hidden disabled={this.disabled} scale={scale} tabIndex={-1}> Why disabled? Scale as well? These are already props.
5. combobox-item-group: <Host scale={scale}> Why scale here?
6. dropdown-group: <Host role="menu" scale={scale} title={this.groupTitle}> Why title & scale?
7. dropdown-item: <Host aria-checked={itemAria} dir={dir} isLink={this.href} role={itemRole} scale={scale} selection-mode={this.selectionMode} tabindex="0"> Why isLink, scale, selectionMode?
8. input-message: <Host calcite-hydrated-hidden={hidden} theme={this.theme}>Why theme?
9. modal: <Host aria-modal="true" is-active={this.isActive} role="dialog"> Why is-active?
10. notice: <Host active={this.active} dir={dir}> Why active?
11. radio-button: <Host labeled={!!this.el.textContent}> Why labeled? what is that for?
12. radio-group-item: <Host appearance={appearance} aria-checked={checked.toString()} layout={layout} role="radio" scale={scale}> Why appearnace, layout, scale?
13. slider: <Host id={id} is-range={this.isRange}> Why isRange?
14. tab-title: <Host aria-controls={this.controls} aria-expanded={this.active.toString()}hasText={this.hasText} id={id} role="tab" tabindex={this.disabled ? "-1" : "0"}> Why hasText?
15. value-list-item: <Host data-id={this.guid}> Why data-id?

Relevant Info

• accordion-item (@jcfranco)
• alert (@jcfranco)
• color-picker-swatch (@jcfranco)
• combobox-item (@jcfranco)
• combobox-item-group (@jcfranco)
• dropdown-group
• dropdown-item
• input-message (@caripizza) feat(alert, button, chip, inline-editable, input-message, link, scrim): move theme attribute styles to global.scss #2114
• modal
• notice
• radio-button
• radio-group-item
• slider
• tab-title
• value-list-item

[jcfranco]

Agreed. We should avoid tacking on to the host anything that can be encapsulated. Once we solidify the list of exceptions, we could add a rule to enforce it.

How do we want to divvy up the list?

[paulcpederson]

Agree we should clean these up and be really careful what we put on the host. Most of these are probably going to be for styling purposes as we use selectors like :host([is-active]) for styles. Depending on how far we want to go, many of these could be replaced by styles lower down the element tree attached to a class.

If props really need to be on a host element, we can use reflect: true as that's a cleaner way to get that functionality.

The aria stuff I'd assume is because you want the labels, etc to exist in light dom, but not totally sure.

[driskull]

In most cases, we don't even need to import and return the host if we aren't' setting anything on it. So best bet would be to just not import and use the Host element.

It would definitely help if we had a rule where when host is used we could only set aria prefixed attributes as well as tabIndex.

[driskull]

How do we want to divvy up the list?

@jcfranco : 1-5
@paulcpederson : 6-10
@driskull : 11-15

Sound good?

[paulcpederson]

Is this part of the current 🏃🏻 ?

[driskull]

Good question. Do you think it should be @jcfranco @paulcpederson @julio8a?

[julio8a]

If this is something you guys feel comfortable with completed by the end of the sprint (5/7), Yes.
https://github.com/Esri/calcite-components/projects/8

[bstifle]

thinking Paul Bryan should handle 1-15

[julio8a]

@driskull @jcfranco @paulcpederson

Should I add this to this sprint?

[driskull]

Should I add this to this sprint?

I got 5 of them done but I'm not sure if the rest could be done for this sprint. Maybe safer for next sprint?

[driskull]

Cool. i'll steal them back.

[jcfranco]

1 – 5 installed!

[driskull]

The rest of mine are installed.

We just need #2114 installed.

[caripizza]

#2114 was just merged. How should we verify this one @driskull ?

[jcfranco]

I've got a PR coming that will use custom a ESLint rule to catch cases where banned host attributes/props are used. We can verify with that. cc @julio8a

[caripizza]

Verified on my local master branch by running npm run lint:ts locally. The only error was related to setupTests:

[1] /Users/car11447/Dev/calcite-components/src/tests/setupTests.ts
[1]   9:17  error  Expect must be inside of a test block  jest/no-standalone-expect

x  178 problems (1 error, 177 warnings)

Marking this one verified and closing per Sprint planning conversation.