Fix groupHeader theming properties for DataTable (#6925)

[miqh]

Hello there,

Another pull request, this time aimed at addressing #6925.

A noteworthy side effect of my proposed fix involved updating existing test snapshots for DataTable. In particular, those which used groupBy for row grouping. I can only guess that this was because theme properties relating to groupHeader were never fed through properly.

---

What does this PR do?

Ensures that any theme properties (e.g. https://v2.grommet.io/datatable#dataTable.groupHeader.background) used to customise a DataTable component are incorporated for rendering.

Where should the reviewer start?

Making sure that the test snapshots I updated are within reason.

What testing has been done on this PR?

I used the available DataTable stories in addition to adding an extra test that covers the provision of groupHeader theme properties.

How should this be manually tested?

Any DataTable story that involves groupBy would be affected by this change (i.e. because even the default groupHeader theme properties didn't appear to be passed along).

Do Jest tests follow these best practices?

• screen is used for querying.
• The correct query is used. (Refer to this list of queries)
• userEvent is used in place of fireEvent.
• asFragment() is used for snapshot testing.

Any background context you want to provide?

None.

What are the relevant issues?

#6925

Screenshots (if appropriate)

This should now be the default appearance when using groupBy:

Edited the custom theme story for DataTable to demonstrate groupHeader theme properties being changed:

Do the grommet docs need to be updated?

Unsure. Maintainers to decide.

Should this PR be mentioned in the release notes?

Likely, as it involves a visual change that would immediately affect anybody using groupBy with DataTable.

Is this change backwards compatible or is it a breaking change?

As mentioned above, this change will alter the default look of DataTable when using groupBy, but it's in the interest of allowing people the ability to customise the appearance of row groups if they don't want the default.

[jcfilben]

Nice work, I'm thinking since previously the dataTable.groupHeader content defined in the base theme was not being used let's change it to:

groupHeader: {
//  background: undefined,
//  border: undefined,
//  pad: undefined
},

This way people using grouped DataTable will be able to customize this but it won't change by surprise when this change gets released.

[MikeKingdom]

Like you hint at in the description, for backward compatibility, I think this needs to fallback to use cellProps.body like before if the groupHeader props aren't defined.

[miqh]

@jcfilben, regarding your suggested adjustment:

groupHeader: {
//  background: undefined,
//  border: undefined,
//  pad: undefined
},

This won't preserve existing style compatibility.

The suggested adjustment would instead yield something like the following (take note of the misaligned expansion buttons):

The pad member needs to be set to the same default as the body context (as hinted to by @MikeKingdom as well) because this what's currently passed through.

In any case, I've adjusted the default theme settings, updating test snapshots as well.

---

While doing this update, I noticed groupEnd is present in the thematic variables, but there's no documentation regarding its customisation potential (perhaps right so, because it'll need some extra change to support).

https://github.com/grommet/grommet-site/blob/master/src/screens/DataTable.js

Similar to what's happened with groupHeader, the changes (#3146) which introduced groupEnd didn't appear to adjust DataTable accordingly to feed through the correct thematic variables.

If you want this done for this pull request too, I'm able to add more changes.

[miqh]

Hi @jcfilben,

I've been cleaning out some stale pull request contributions and noticed this one.

Based on my last comment, I don't believe any further changes on my part are needed (unless I've missed something).

The groupEnd matter can be raised as a separate issue if need be.

[jcfilben]

Hi @miqh thanks for checking back in on this, I think instead of setting dataTable.groupHeader.pad to { horizontal: 'small', vertical: 'xsmall' } we should leave it as undefined. When it is undefined we should fall back to the styling from cellProps.body.

[miqh]

@jcfilben, I feel a bit silly glossing over the point about cellProps.body as a fallback.

It was raised much earlier too, so I wasn't reading carefully enough.

So, with the fallback to cellProps.body, I've been able to get rid of the default theme value for dataTable.groupHeader.pad and for DataTable itself to preserve its presentation.

Hope that does it.

[jcfilben]

@jcfilben, I feel a bit silly glossing over the point about cellProps.body as a fallback.

It was raised much earlier too, so I wasn't reading carefully enough.

So, with the fallback to cellProps.body, I've been able to get rid of the default theme value for dataTable.groupHeader.pad and for DataTable itself to preserve its presentation.

Hope that does it.

No worries! Thanks for getting these changes in, code looks good!

[MikeKingdom]

LGTM! Thanks!