Skip to content

Conversation

britt6612
Copy link
Collaborator

@britt6612 britt6612 commented May 4, 2022

What does this PR do?

This PR takes out

  @media all and (min--moz-device-pixel-ratio: 0) {
    table-layout: fixed;
  }

This was setting the table-layout to be fixed in firefox which then the table’s layout ignores the content and instead uses the table’s width, any specified width of columns, and border and cell spacing values.

Where should the reviewer start?

styledTable.js

What testing has been done on this PR?

storybook

How should this be manually tested?

import React from 'react';

import { Box, DataTable, Text, Meter } from 'grommet';

const amountFormatter = new Intl.NumberFormat('en-US', {
  style: 'currency',
  currency: 'USD',
  minimumFractionDigits: 2,
});

const columns = [
  {
    property: 'name',
    header: <Text>Name with extra</Text>,
    primary: true,
    footer: 'Total',
  },
  {
    property: 'location',
    header: 'Location',
    // size: 'small',
    render: ({ location }) => <Text truncate>{location}</Text>,
  },
  {
    property: 'date',
    header: 'Date',
    render: (datum) => <Text truncate>{datum.date}</Text>,
    align: 'end',
  },
  {
    property: 'percent',
    header: 'Percent Complete',
    render: ({ percent }) => (
      <Box pad={{ vertical: 'xsmall' }}>
        <Meter values={[{ value: percent }]} thickness="small" size="small" />
      </Box>
    ),
  },
  {
    property: 'paid',
    header: 'Paid',
    render: (datum) => amountFormatter.format(datum.paid / 100),
    align: 'end',
    aggregate: 'sum',
    footer: { aggregate: true },
  },
];

const groupColumns = [...columns];
const first = groupColumns[0];
groupColumns[0] = { ...groupColumns[1] };
groupColumns[1] = { ...first };
groupColumns[0].footer = groupColumns[1].footer;
delete groupColumns[1].footer;

const DATA = [
  {
    name: 'Alan',
    location: '',
    date: '',
    percent: 0,
    paid: 0,
  },
  {
    name: 'Bryan',
    location: 'Fort Collins long long long long',
    date: '2018-06-10',
    percent: 30,
    paid: 1234,
  },
  {
    name: 'Chris',
    location: 'Palo Alto',
    date: '2018-06-09',
    percent: 40,
    paid: 2345,
  },
  {
    name: 'Eric',
    location: 'Palo Alto',
    date: '2018-06-11 really really really long',
    percent: 80,
    paid: 3456,
  },
  {
    name: 'Doug',
    location: 'Fort Collins',
    date: '2018-06-10',
    percent: 60,
    paid: 1234,
  },
  {
    name: 'Jet',
    location: 'Palo Alto',
    date: '2018-06-09',
    percent: 40,
    paid: 3456,
  },
  {
    name: 'Michael',
    location: 'Boise',
    date: '2018-06-11',
    percent: 50,
    paid: 1234,
  },
  {
    name: 'Tracy',
    location: 'San Francisco',
    date: '2018-06-10',
    percent: 10,
    paid: 2345,
  },
];

export const Simple = () => (
  // Uncomment <Grommet> lines when using outside of storybook
  // <Grommet theme={grommet}>
  <Box height="medium" width="600px" overflow="auto">
    <DataTable columns={groupColumns} data={DATA} />
  </Box>
  // </Grommet>
);

export default {
  title: 'Visualizations/DataTable/Simple',
};

Use the code above in a story and see that in FireFox it was a fixed width for each cell which should not be the right behavior without setting size we want the cells to grow with the content

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?

The

  @media all and (min--moz-device-pixel-ratio: 0) {
    table-layout: fixed;
  }

was put in as a fix for

If Table is wrapped in (Box with a width limit) and is inside this table, then in Firefox it will go beyond the limits of this Box. The problem is found only in Firefox - in Chrome and in Safari everything works fine.
#2656
FireFox was not respecting the width however this was 3 years ago and things have changed this is no longer an issue and FF respects the width that is inherited.
This can be tested with the code below in FireFox


   <Box width="small" border>
      <Table caption="Simple Table">
        <TableBody>
          <TableRow>
            <TableCell>
              <Meter
                type="bar"
                values={[
                  {
                    value: 60
                  }
                ]}
              />
            </TableCell>
            <TableCell>
              <Text>Text</Text>
            </TableCell>
          </TableRow>
        </TableBody>
      </Table>
    </Box>

What are the relevant issues?

closes #6081

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Im thinking we should update the size prop for Datatable in the docs it states now it only effects the height however when size is set it adds

    display: table;
    width: 100%;
    table-layout: fixed;

Should this PR be mentioned in the release notes?

sure

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

backwards compatible

@britt6612 britt6612 requested a review from jcfilben May 4, 2022 02:42
package.json Outdated
@@ -86,7 +86,7 @@
"babel-core": "^7.0.0-bridge.0",
"babel-jest": "^27.5.1",
"babel-loader": "^8.2.5",
"babel-plugin-styled-components": "^2.0.7",
"babel-plugin-styled-components": "2.0.6",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be changing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I have it in another PR Ill strip this out

Copy link
Collaborator Author

@britt6612 britt6612 May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was strange changes in snapshots made so I opened this PR
#6110

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taken out

yarn.lock Outdated
@@ -3541,7 +3541,18 @@ babel-plugin-react-docgen@^4.2.1:
lodash "^4.17.15"
react-docgen "^5.0.0"

"babel-plugin-styled-components@>= 1.12.0", babel-plugin-styled-components@^2.0.7:
babel-plugin-styled-components@2.0.6:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yarn.lock shouldn't change either

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it got merged into master

@britt6612 britt6612 requested a review from taysea May 4, 2022 23:42
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I agree we should update the size prop description in the docs

Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@taysea taysea requested a review from ericsoderberghp May 5, 2022 22:08
@ericsoderberghp ericsoderberghp merged commit 5b48077 into grommet:master May 5, 2022
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.

datatable truncation not working in chrome
4 participants