Implement table headerless column

[shahsnidhi]

Closes
#912

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Add a hideHeader prop to the column. This will

1. Not add a textual header to the column
2. Tooltip will be added instead of the text
3. The column will have a default fixed width depending on the scale

🧢 Your Project:

[snowystinger]

Thanks for getting this going!

[shahsnidhi]

@snowystinger Made some changes as discussed.

[snowystinger]

Looking really good, couple last small things

[snowystinger]

curious, why is clip needed?

[shahsnidhi]

Here I am overriding text-overflow: ellipsis in https://github.com/adobe/react-spectrum/blob/main/packages/@adobe/spectrum-css-temp/components/table/index.css#L156 with default as we wouldn't want to add ellipsis hideHeader column

[snowystinger]

does it affect something if we don't touch it? we wouldn't render anything in there right?

[shahsnidhi]

This is the hideHeader column content, so we would have the action button in there.
In absence of this style, i see ellipsis getting added when a showDivider props is added to the column. I am going to make some changes to the styles of hideHeader cell and contents and the defaultWidth so we don't need to default this.
Let me know if there is a better solution

[shahsnidhi]

adds a px to support the divider

[snowystinger]

What are the implications of this? if there is no divider, is it still ok?

[shahsnidhi]

Yes

[snowystinger]

What if we kept the width at the right amount, and added 1px for the columns with a divider?

    getDefaultWidth: (column) => {
      let {hideHeader, isSelectionCell} = column;
      let width;
      if (hideHeader) {
        width = DEFAULT_HIDE_HEADER_CELL_WIDTH[scale];
      } else if (isSelectionCell) {
        width = SELECTION_CELL_DEFAULT_WIDTH;
      }
      if (column.showDivider && !isNaN(width)) {
        width += 1;
      }
      return width;
    }

[snowystinger]

The screenshots don't quite look right, the focus ring around a button shouldn't be clipped like that
I think we still need https://github.com/adobe/react-spectrum/blob/main/packages/%40adobe/spectrum-css-temp/components/table/index.css#L158 and I think we might actually need it to be

.spectrum-Table-cellContents--hideHeader {
  /* allow focus ring of child to extend outside the bounds */
  padding: 4px;
  margin: -4px;
}

I suggest this as an alternative because this helps the clipping at the top and bottom

I think it also needs something closer to this:

const DEFAULT_HIDE_HEADER_CELL_WIDTH = {
  medium: 36,
  large: 44
};

but i don't know where those values were originally pulled from
I still have some clipping on the left with all the above changes, but I missed editing something you've already thought of. Any ideas?

[shahsnidhi]

So the focus ring around a button is different than a focus ring around textual content check out the table story for crud and focus around the actions /?path=/story/table--crud
I believe that's the expected behavior. So we don't need the negating margin padding combo for this

The width of the button icon was taking into consideration the width of actionButton ie.
32 and 40 (based on scale).
The button has a border 1 px so adding 2 px to the overall width
34 and 42 (based on scale)
plus the divider 1px
35 and 43 (based on scale)

If we add padding of 4px we will have to increase the width by 8px to accommodate the added pxs

[snowystinger]

lets adjust the names to match the rest, the other references will need to be adjusted as well

[majornista]

Since TooltipTrigger should not add aria-describedby until the Tooltip is open, we should add the rendered column header content within a VisuallyHidden element:

        {columnProps.hideHeader ?
          <VisuallyHidden>{column.rendered}</VisuallyHidden> :
          column.rendered
        }

[majornista]

Related to this, TooltipTrigger is always adding aria-describedby regardless of whether the state.isOpen or not.

At:

react-spectrum/packages/@react-aria/tooltip/src/useTooltipTrigger.ts

Line 135 in 975957a

'aria-describedby': state.open ? tooltipId : undefined,

state.open is a function, so it's always true, whereas state.isOpen is a boolean, this line should be:

      'aria-describedby': state.isOpen ? tooltipId : undefined,

[majornista]

I've opened a related issue: #1149.

[shahsnidhi]

Made the required changes
cc: @snowystinger @majornista

[majornista]

#1150 fixes this issue and includes a unit test.

[snowystinger]

can you add a comment to explain this one and why it's different from the other one?

[shahsnidhi]

So looks like the content is not center aligned and this padding helps to align the content
I can also fix the issue of left focus ring getting cut off with

.spectrum-Table-cell--hideHeader {
   padding: 0; // default padding for other cells is around 16px, so we need to override that
  justify-content: center;
}

The same happens to all the buttons in the table when we remove the padding around them and also remove the alignment. checkout the CRUD example
Need your inputs on which method would be better? existing OR above, considering the other pieces in the table

[snowystinger]

setting padding to 0 and justifying center should be the better way to do it i think

[shahsnidhi]

Agreed, we should be good to go now