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

Add data table #281

Merged
merged 58 commits into from
Aug 29, 2023
Merged

Add data table #281

merged 58 commits into from
Aug 29, 2023

Conversation

balakrishnan-s5
Copy link
Contributor

@balakrishnan-s5 balakrishnan-s5 commented Aug 15, 2023

Changes

  • Added component data table

    [Delete] branch

Additional information

@balakrishnan-s5 balakrishnan-s5 self-assigned this Aug 15, 2023
@balakrishnan-s5 balakrishnan-s5 changed the base branch from master to data-table-long-lived August 16, 2023 04:28
Copy link
Contributor

@qroll qroll left a comment

Choose a reason for hiding this comment

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

some initial comments on the api

src/data-table/types.ts Show resolved Hide resolved
src/data-table/types.ts Outdated Show resolved Hide resolved
src/data-table/types.ts Outdated Show resolved Hide resolved
src/data-table/types.ts Outdated Show resolved Hide resolved
stories/data-table/data-table.stories.mdx Outdated Show resolved Hide resolved
src/data-table/types.ts Outdated Show resolved Hide resolved
src/data-table/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

more of a DX question, can take this back for discussion with RBS and your team since you guys will be consuming this component

would devs prefer to work with separate props to configure header and row (current state)

or have a combined config e.g.

columns={[
      {
        header: "Status", // or ReactNode
        fieldKey: "status", // maps to the object property
        sort: "asc", // or "desc"
        onHeaderClick: () => {},
        renderRowCell: (value, item, renderProps) => {},
      }
]}

Copy link
Contributor Author

@balakrishnan-s5 balakrishnan-s5 Aug 18, 2023

Choose a reason for hiding this comment

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

As discussed we will keep the header and row configs separated

Copy link
Contributor

Choose a reason for hiding this comment

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

Still find the current approach awkward. Puts alot of onus on the user to make sure they specify the matching number of header cols and row cols. Let me think more if there is a better way

@balakrishnan-s5 balakrishnan-s5 requested a review from qroll August 18, 2023 04:59
@qroll qroll changed the base branch from data-table-long-lived to master August 21, 2023 08:36
@qroll qroll changed the base branch from master to data-table-long-lived August 21, 2023 08:36
src/data-table/data-table.styles.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@qroll qroll Aug 22, 2023

Choose a reason for hiding this comment

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

  • is it intended for the non-first cells to not have horizontal padding? they are misaligned with the headers which do have padding
Screen.Recording.2023-08-22.at.9.31.51.AM.mov

Copy link
Contributor

@qroll qroll Aug 22, 2023

Choose a reason for hiding this comment

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

  • similarly the loader and empty image are not centered

Screenshot 2023-08-22 at 9 38 34 AM

Copy link
Contributor

@qroll qroll Aug 22, 2023

Choose a reason for hiding this comment

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

  • the last column should have the additional right padding as well to match the first column

Screenshot 2023-08-22 at 9 39 23 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

  • the last column should have a right padding
Screenshot 2023-08-28 at 11 41 34 AM


const sortIconStyles = css`
color: ${Color.Neutral[1]};
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed as there's no font weight for icons

Copy link
Contributor

Choose a reason for hiding this comment

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

On this same note, if the styles applied are the same for both icons, can consider adding it in the parent element instead and use the svg selector

svg {
     // your styles here...
}

src/data-table/data-table.styles.tsx Outdated Show resolved Hide resolved
src/data-table/data-table.styles.tsx Outdated Show resolved Hide resolved
src/data-table/types.ts Outdated Show resolved Hide resolved

const basicEmptyView = () => {
return (
<ErrorDisplay
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a default title/description?

also note that the ErrorDisplay currently doesn't allow empty title/description (it will revert to the placeholder copy). if this functionality is needed, please make the change in a separate PR

separately, the header in ErrorDisplay is H1 but Figma shows H3 for the table. can check with UX if this is ok, otherwise you can override the text styling


const sortIconStyles = css`
color: ${Color.Neutral[1]};
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

On this same note, if the styles applied are the same for both icons, can consider adding it in the parent element instead and use the svg selector

svg {
     // your styles here...
}

src/data-table/data-table.tsx Outdated Show resolved Hide resolved
@balakrishnan-s5 balakrishnan-s5 requested a review from qroll August 25, 2023 05:35
Copy link
Contributor

@qroll qroll left a comment

Choose a reason for hiding this comment

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

noting down the outstanding items that should be covered separately:

  • props changes and error display enhancements (follow up PR)
  • style changes to match Figma (part 2 PR)
  • any new features (part 3 PR)
  • storybook documentation (final PR if needed)

max-width: ${(props) => (props.$maxWidth ? props.$maxWidth : "auto")};
vertical-align: middle;
${TextStyleHelper.getFontFamily("H5", "bold")}
color: ${Color.Neutral[1]}
Copy link
Contributor

Choose a reason for hiding this comment

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

to add the semicolon here

justify-content: center;
`;

export const CheckBox = styled(Checkbox)``;
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed if no special styling is required

Copy link
Contributor

Choose a reason for hiding this comment

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

  • the last column should have a right padding
Screenshot 2023-08-28 at 11 41 34 AM

align-items: center;
`;

export const EmptyDataElement = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be unused

Comment on lines 172 to 176
div {
h1 {
${TextStyleHelper.getFontFamily("H3", "bold")}
}
}
Copy link
Contributor

@qroll qroll Aug 28, 2023

Choose a reason for hiding this comment

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

not ideal way to style the text elements but will take this for now. let's clarify with UX if h3 is the standard size for an inline error display. we can introduce a prop like displayType: "default" | "small" as part of the follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll remove the font size and weight change. Follow up URL, I'll get update from UX and do the changes.

@qroll qroll merged commit cab1a80 into data-table-long-lived Aug 29, 2023
1 check passed
@qroll qroll deleted the add-data-table branch August 29, 2023 01:02
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.

4 participants