Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed DataTable to allow setting the pinned background from the background context #4801

Merged
merged 7 commits into from
Feb 10, 2021

Conversation

ericsoderberghp
Copy link
Member

What does this PR do?

Changed DataTable to allow setting the pinned background from the background context

Where should the reviewer start?

Box.js
Cell.js

What testing has been done on this PR?

Added unit tests, tweaked Fill and Pin storybook

How should this be manually tested?

storybook

What are the relevant issues?

grommet/grommet-theme-hpe#145

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

yes

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

Not 100%. If a theme sets a pinned background, there is no change. Using the grommet theme, currently pinned DataTable elements have no background, which doesn't look very good. This case will look different as we will use the background color + opacity.

@halocline
Copy link
Collaborator

halocline commented Dec 5, 2020

I am finding myself wanting to articulate the hierarchy / cascading of the background colors. Let me see if I have this correct.

Working from acute to general:

  • <DataTable background={{ header: ..., body: ..., footer: ... }} />
  • <DataTable background="..." />
  • theme.dataTable.pinned[context].background.color
  • theme.table.[header, body, footer].background.color
  • background from Box background prop via ThemeContext.Provider

If I have that correct, I think I like the direction. Background context gets established by a Box's background prop. I am curious where you have reservations.

Additionally, and again if I have this hierarchy correct, I think the current implementation is not taking into consideration any background defined in theme.table.[header, body, footer].

@ericsoderberghp
Copy link
Member Author

I am finding myself wanting to articulate the hierarchy / cascading of the background colors. Let me see if I have this correct.

Working from acute to general:

  • <DataTable background={{ header: ..., body: ..., footer: ... }} />
  • <DataTable background="..." />
  • theme.dataTable.pinned[context].background.color
  • theme.table.[header, body, footer].background.color
  • background from Box background prop via ThemeContext.Provider

If I have that correct, I think I like the direction. Background context gets established by a Box's background prop. I am curious where you have reservations.

Additionally, and again if I have this hierarchy correct, I think the current implementation is not taking into consideration any background defined in theme.table.[header, body, footer].

I think you're on the right track. But, from Table's point of view, theme.dataTable.* becomes <Table background />. So, if the DataTable theme doesn't specify background colors, then the theme.table.* backgrounds will be used.

@ericsoderberghp ericsoderberghp marked this pull request as ready for review January 25, 2021 22:23
@ShimiSun ShimiSun merged commit 1c1b69d into master Feb 10, 2021
@ShimiSun ShimiSun deleted the fix/datatable-pinned-background branch February 10, 2021 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants