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
feat(tools): Implement basic functionality of table component #47263
Conversation
👀 Review this PR in a CodeSee Review Map |
I see some failing tests here - I think those need to be looked at. |
1838df3
to
e59b06f
Compare
e59b06f
to
25141ba
Compare
Tests all taken care of, ready for review |
Some of the props such as |
@ahmadabdolsaheb I've fixed the What's left is handling use cases for when tables have the props |
41991d9
to
1718f8d
Compare
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: |
@ahmadabdolsaheb I sent you a discord message with the problems I was facing |
@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 Let me know if that makes sense. |
Hey @ahmadabdolsaheb , I think I may be a little confused. Are we still utilizing bootstrap CSS? For instance, if I want to use Otherwise, when I test the Here are a few screenshots of the table in Storybook: When I tried to do what you suggested, the styles were not being applied to 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(' ');
}; |
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 { 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). 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.
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. |
@JordanMooree, how is the table component development progressing on your end. Let me know if you need any help. |
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 if (striped)
classNames.push('[&>tbody>tr:nth-of-type(odd)]:bg-background-tertiary'); When I tried to simplify it to I need help running the |
Thank you for your patience with the review. I will look into the tailwind config, debug it, and let you know how to proceed. |
Took a final look. This looks solid. |
Checklist:
Update index.md
)main
branch of freeCodeCamp.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):