freeCodeCamp icon indicating copy to clipboard operation
freeCodeCamp copied to clipboard

feat(tools): Implement basic functionality of table component

Open JordanMooree opened this issue 2 years ago • 15 comments

Checklist:

  • [x] I have read freeCodeCamp's contribution guidelines.
  • [x] My pull request has a descriptive title (not a vague title like Update index.md)
  • [x] My pull request targets the main branch of freeCodeCamp.
  • [x] I have tested these changes either locally on my machine, or GitPod.

Ref #47232

Tested this with the table components on the timeline on the user profile and user settings under certifications. Both look and perform as expected. I still need to (will address these in separate PRs):

  • Create tests
  • Address a11y for role, property, and tabindex attributes
  • Address the hover background colors for light and dark mode

JordanMooree avatar Aug 11 '22 19:08 JordanMooree

gitpod-io[bot] avatar Aug 11 '22 19:08 gitpod-io[bot]

:eyes: Review this PR in a CodeSee Review Map

View the CodeSee Map of this change

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

ghost avatar Aug 11 '22 19:08 ghost

I see some failing tests here - I think those need to be looked at.

moT01 avatar Aug 12 '22 14:08 moT01

Tests all taken care of, ready for review

JordanMooree avatar Aug 12 '22 20:08 JordanMooree

Some of the props such as stripped and size are not getting applied. Additionally, I believe we are missing the condensed prop. The component is coming together nicely.

ahmaxed avatar Aug 15 '22 10:08 ahmaxed

@ahmadabdolsaheb I've fixed the size and striped props. I've also added the condensed prop as well.

What's left is handling use cases for when tables have the props variant = 'light' | 'dark' and striped and hover set to true and figuring out the appropriate background color to apply when hovering over striped tr elements in dark or light variant tables.

JordanMooree avatar Aug 15 '22 20:08 JordanMooree

Thanks for working on this component, it is coming together pretty well.

Initially only the props used in learn should be implemented(condensed, striped). On the upcoming iterations, we will add the extra functionalities as needed.

Our custom palette should be used for colors. That way the colors will switch automatically when the theme changes. You could use other newly developed components as a reference.

Lastly, since the table children are all getting a padding, nesting elements would result in double padding.

Here is a comparison:

Screen Shot 2022-08-23 at 5 33 38 PM

ahmaxed avatar Aug 23 '22 14:08 ahmaxed

@ahmadabdolsaheb I sent you a discord message with the problems I was facing

JordanMooree avatar Aug 29 '22 23:08 JordanMooree

@JordanMooree, We could simplify the following styles to

.table-condensed > thead > tr > th,
.table-condensed > tbody > tr > th,
.table-condensed > tfoot > tr > th,
.table-condensed > thead > tr > td,
.table-condensed > tbody > tr > td,
.table-condensed > tfoot > tr > td {
  padding: 5px;
}

.table > thead > tr > th,
.table > tbody > tr > th,
.table > tfoot > tr > th,
.table > thead > tr > td,
.table > tbody > tr > td,
.table > tfoot > tr > td {
  padding: 8px;
/* rest of the styles*/
}

To

.table th, .table td {
  padding: 8px;
/* rest of the styles*/
}

.table-condensed th, .table-condensed td {
 padding: 5px
}

And use arbitrary variants to set the styles without using addVariant

Let me know if that makes sense.

ahmaxed avatar Sep 06 '22 13:09 ahmaxed

Hey @ahmadabdolsaheb , I think I may be a little confused. Are we still utilizing bootstrap CSS? For instance, if I want to use .table-condensed should I just push table-condensed class to the Table component I'm working on?

Otherwise, when I test the Table Component on the timeline, styles are not being applied (I think it may be due to bootstrap overriding styles) but when I test the component on Storybook, everything seems to be working fine.

Here are a few screenshots of the table in Storybook:

Default: image

Condensed: image

Striped: image

When I tried to do what you suggested, the styles were not being applied to th and td since they were nested in tbody or thead.

My current implementation:

const computeClassNames = ({
  condensed,
  striped
}: {
  condensed: boolean;
  striped: boolean;
}) => {
  const classNames = [...defaultClassNames];
  if (condensed)
    classNames.push(
      '[&>thead>tr>th]:p-1 [&>tbody>tr>th]:p-1 [&>tfoot>tr>th]:p-1 [&>thead>tr>td]:p-1 [&>tbody>tr>td]:p-1 [&>tfoot>tr>td]:p-1'
    );
  else
    classNames.push(
      '[&>thead>tr>th]:p-2 [&>tbody>tr>th]:p-2 [&>tfoot>tr>th]:p-2 [&>thead>tr>td]:p-2 [&>tbody>tr>td]:p-2 [&>tfoot>tr>td]:p-2'
    );
  if (striped)
    classNames.push('[&>tbody>tr:nth-of-type(odd)]:bg-background-tertiary');

  return classNames.join(' ');
};

JordanMooree avatar Sep 06 '22 17:09 JordanMooree

Thanks for your patience with the review.

Although bootstrap classes are further restyled in the learn platform, the component library's styles should not depend on those styles. Once we import the components one by one, we will remove those custom styles.

We need to build the component library before using it; However, I see there is are a few issues with the build process as it does import the colors and the built css errors out when imported directly. For now, you could drop import './base.css'; in tools/ui-components/src/index.ts and npm run build. Once the build is finished, import the bundle file in your desired learn component. Here is the snippet for importing the bundle in the time-tine.tsx component

import { Table } from '../../../../../tools/ui-components/dist/bundle.esm';

If you visit the profile page locally and look into the dev console you will see that the paddings and background colors are being applied. Background colors don't show up since color variables are not defined(don't worry about that).

Screen Shot 2022-09-15 at 11 38 58 AM

For the arbitrary variants, you could simplify the styles to the followings:

    if (condensed) classNames.push('[&_td]:p-1 [&_th]:p-1');
    else classNames.push('[&_td]:p-2 [&_th]:p-2');

You could apply simplify the styles for the striped variation and use arbitrary variants accordingly.

I noticed that I get the following ts error when importing the table component from the ui component library to the time-line.tsx component.

Type '{ children: Element[]; condensed: boolean; striped: boolean; }' is not assignable to type 'IntrinsicAttributes & RefAttributes<any>'.
  Property 'children' does not exist on type 'IntrinsicAttributes & RefAttributes<any>'.

It is probably telling us that there is a prop type. It would be great if we could take a look at that as well.

Thank you for all of your efforts. This component is coming up really good. Let me know if you have other questions.

ahmaxed avatar Sep 15 '22 08:09 ahmaxed

@JordanMooree, how is the table component development progressing on your end. Let me know if you need any help.

ahmaxed avatar Sep 30 '22 13:09 ahmaxed

Hey @ahmadabdolsaheb, sorry for the delay I am about to push some changes I made to the arbitrary variants with the suggestions you made. For striped variation, I could not simplify the styles because I needed to select tbody like this:

if (striped)
    classNames.push('[&>tbody>tr:nth-of-type(odd)]:bg-background-tertiary');

When I tried to simplify it to &_tr:nth-of-type(odd) it also selects table rows in the thead tag.

I need help running the npm run build command because it doesn't create the bundle for some reason (maybe because I am using Gitpod? Should I set up FCC locally?). I followed the directions you provided but it didn't work for me so I wasn't able to inspect that error either

JordanMooree avatar Oct 02 '22 18:10 JordanMooree

Thank you for your patience with the review. I will look into the tailwind config, debug it, and let you know how to proceed.

ahmaxed avatar Oct 16 '22 12:10 ahmaxed

Took a final look. This looks solid. Not sure why build is not working on Gitpod, I can test things on my side if you cannot set up the repo locally. Looking forward to the Table related prs. 🙌

ahmaxed avatar Oct 27 '22 14:10 ahmaxed