Enhancement: Tip - Remove use of data attribute for group title

[esri]

Enhancement: Tip - Remove use of data attribute for group title

[pr3tori4n]

@driskull This sounds like it would add a regression bug. Currently, the desired behavior the entire tip manager doesn't have a single title, but that the tips within the manager can be "grouped" by sharing a common title for some, and a common title for others. This was a feature requested by @asangma.
See Bullet 2

I'm open to improvements to the implementation details - but we'll need a solution that maintains the current functionality.

[driskull]

This is not a regression, it is changing from a data-attribute to a custom element attribute.

[driskull]

Proposed change

• Create a calcite-tip-group component
• This avoids declaring the group title multiple times.

<calcite-tip-manager>
  <calcite-tip-group text-title="My Group">
    <calcite-tip></calcite-tip>
    <calcite-tip></calcite-tip>
  <calcite-tip-group>
</calcite-tip-manager>

Which would follow practices of native elements like select

<select>
  <optgroup label="Swedish Cars">
    <option value="volvo">Volvo</option>
    <option value="saab">Saab</option>
  </optgroup>
  <optgroup label="German Cars">
    <option value="mercedes">Mercedes</option>
    <option value="audi">Audi</option>
  </optgroup>
</select>

[pr3tori4n]

Ok at first I thought you wanted to elevate it to the manager. We're keeping it on the tip, just changing the name and adding a prop.

[driskull]

I think we should make a decision on using a group vs declaring it on multiple tips because this will affect other widgets and how they are architected.

[pr3tori4n]

👍 Let's discuss in our sync in a few.

[driskull]

Another reason why the group title doesn't belong on the tip is because the tip doesn't render it. Its only rendered on the TipManager. So it makes more sense on a group element that can be tied to the TipManager and would only work with the TipManager.

[pr3tori4n]

By that logic, wouldn't that also preclude it from belonging on the groupElement? The group element doesn't render the title either.

[pr3tori4n]

Generally in my experience, avoiding unneccessary DOM structure saves hassle.

Here's an example of where adding wrapping elements can monkey with flexbox.
https://codepen.io/pr3tori4n/pen/xoeaBr

Extra DOM Also affects direct child selectors and sibling selectors in CSS.
Additionally it would affect inline or inline-block layout behavior. If the child elements were inline-block, the wrapper would have to be also.

It would also impact the mutation observer, requiring us to listen to changes in the subtree.

[driskull]

By that logic, wouldn't that also preclude it from belonging on the groupElement? The group element doesn't render the title either.

Nope, because we could document the group element as an optional part of creating a TipManager.

The TipManager would render tips or a group of tips.

avoiding unneccessary DOM structure saves hassle.

I wouldn't say its unnecessary. Its semantically necessary.

Extra DOM Also affects direct child selectors and sibling selectors in CSS.
Additionally it would affect inline or inline-block layout behavior. If the child elements were inline-block, the wrapper would have to be also.

I don't see how this is an issue in this use case. Is this relevant to this change?

It would also impact the mutation observer, requiring us to listen to changes in the subtree.

Yeah, it adds some extra logic but I think its worth it.

[driskull]

If this isn't going to make it into V1 lets...

• Remove the data attribute
• Do not document any way to set the group title and it will only ever show "Tips" for V1.

V2...

• Introduce <calcite-tip-group text-title="My Group">

[pr3tori4n]

@jcfranco did you sync with Matt last week regarding this? Currently there's a wrinkle - the tip manager CSS is modifying the tips border and box shadow, as well as controlling the hiding/showing of the selected tip. This CSS no longer works if we add a wrapping component, since the ::slotted() selector is only able to target elements 1 level inside a slot.

A possible solution would be to use the hidden attribute instead of "selected" to control hide/show. But what should be done about the tip's normal boundary? We could add a prop to tip which toggles this, but should that be added by the tip manager, or should that be something expected of whoever is implementing this?

[driskull]

A possible solution would be to use the hidden attribute instead of "selected" to control hide/show. But what should be done about the tip's normal boundary? We could add a prop to tip which toggles this, but should that be added by the tip manager, or should that be something expected of whoever is implementing this?

Would hidden and selected handle both of the issues?

hidden would hide the ones that don't need to be shown and selected could handle the styling differences.

Currently, the TipManager is setting selected on the Tip but that is not actually a valid documented property.

[pr3tori4n]

Would hidden and selected handle both of the issues?

No. The tip manager has these lines of CSS. Currently adding selected handles both cases. If we introduce a new wrapping component (e.g. ) as suggested, there's no CSS we can add to the tip manager that can affect the tips. In essence, the tip manager would lose any stylistic control it currently has over tips.

This problem is avoided if we continue to rely on an attribute for grouping tips. It's worth calling out this is how radio buttons and checkboxes work as a contrasting example to a select, relying on a common name attribute.

If we proceed with the new custom element as a solution to grouping, we could get around this if we move the CSS that's currently in the tip manager styles to the tip styles. Hide/show already exists for [hidden]. We'd need some other style modifier and a new prop to activate it to create a modified tip style. As an example:
@Prop() noBoundary = false;

:host[noBoundary] {
  box-shadow: none;
  max-width: $tip-max-width;
  width: $tip-width;
  margin: 0;
  padding: 0;
}

What's your preference?

[driskull]

In essence, the tip manager would lose any stylistic control it currently has over tips.

I think that is ok. If you add a @Prop selected to the Tip, you can use it to style the Tip within the Tip's CSS instead of from the TipManager. (I think this is better)

Currently, you are adding a selected attribute on the Tip, but that is not a valid documented property.

we could get around this if we move the CSS that's currently in the tip manager styles to the tip styles.

Yep!

@prop() noBoundary = false;

Can't you just use selected???? If you're using a stand-alone Tip, then you don't need to set selected and it would not get the styles.

[driskull]

This problem is avoided if we continue to rely on an attribute for grouping tips. It's worth calling out this is how radio buttons and checkboxes work as a contrasting example to a select, relying on a common name attribute.

That is comparing apples and oranges. The name attribute is for referencing, not grouping or sharing a common text value.

Its actually more like a FieldSet/Legend...

<fieldset name="test">
  <legend>Personalia:</legend>
  Name: <input type="text"><br>
  Email: <input type="text"><br>
  Date of birth: <input type="text">
 </fieldset>

[jcfranco]

@pr3tori4n Do you think the selected approach would work? I'd suggest proceeding with this work, not doc group title to move forward with v1-alpha.0. IMO, we can squeeze this in if not disruptive in an alpha update.

[pr3tori4n]

This has been merged. @driskull please verify.