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

feat(tools): Implement basic functionality of table component #47263

Merged
merged 9 commits into from Nov 2, 2022

Conversation

JordanMooree
Copy link
Sponsor Member

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • 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

@gitpod-io
Copy link

gitpod-io bot commented Aug 11, 2022

@github-actions github-actions bot added the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Aug 11, 2022
@JordanMooree JordanMooree mentioned this pull request Aug 11, 2022
2 tasks
@ghost
Copy link

ghost commented Aug 11, 2022

👀 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

@moT01 moT01 added the status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP label Aug 12, 2022
@moT01
Copy link
Member

moT01 commented Aug 12, 2022

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

@JordanMooree JordanMooree force-pushed the tools/table-component branch 5 times, most recently from 1838df3 to e59b06f Compare August 12, 2022 19:00
@JordanMooree JordanMooree added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Aug 12, 2022
@JordanMooree
Copy link
Sponsor Member Author

Tests all taken care of, ready for review

@ahmaxed
Copy link
Member

ahmaxed commented Aug 15, 2022

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.

@JordanMooree
Copy link
Sponsor Member Author

JordanMooree commented Aug 15, 2022

@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 JordanMooree marked this pull request as draft August 16, 2022 20:31
@JordanMooree JordanMooree added the status: PR in works Work in Progress (WIP) Issues. label Aug 16, 2022
@raisedadead raisedadead removed the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label Aug 22, 2022
@ahmaxed ahmaxed self-assigned this Aug 22, 2022
@ahmaxed
Copy link
Member

ahmaxed commented Aug 23, 2022

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

@JordanMooree
Copy link
Sponsor Member Author

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

@ahmaxed
Copy link
Member

ahmaxed commented Sep 6, 2022

@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.

@JordanMooree
Copy link
Sponsor Member Author

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(' ');
};

@ahmaxed
Copy link
Member

ahmaxed commented Sep 15, 2022

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
Copy link
Member

ahmaxed commented Sep 30, 2022

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

@JordanMooree
Copy link
Sponsor Member Author

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

@ShaunSHamilton ShaunSHamilton removed their request for review October 6, 2022 18:39
@ahmaxed
Copy link
Member

ahmaxed commented Oct 16, 2022

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
Copy link
Member

ahmaxed commented Oct 27, 2022

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 ahmaxed marked this pull request as ready for review October 27, 2022 14:32
@raisedadead raisedadead added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: PR in works Work in Progress (WIP) Issues. labels Nov 2, 2022
@raisedadead raisedadead merged commit d5d03e0 into freeCodeCamp:main Nov 2, 2022
@JordanMooree JordanMooree deleted the tools/table-component branch November 2, 2022 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants