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: Create basic table component #3

Merged
merged 25 commits into from
Jul 29, 2024

Conversation

delemaf
Copy link
Contributor

@delemaf delemaf commented Jun 27, 2024

📌 References

📝 Implementation

  • Create BasicTable component
  • Handle simple text, link and select with custom options
  • Handle index column

📹 Screenshots/Screen capture

image

image

🔥 Notes to the tester

@delemaf delemaf added the enhancement New feature or request label Jun 27, 2024
@delemaf delemaf self-assigned this Jun 27, 2024
@delemaf delemaf changed the title feat: create basic table component feat: Create basic table component Jun 28, 2024
@delemaf delemaf requested a review from 9sneha-n June 28, 2024 07:36
Copy link
Contributor

@9sneha-n 9sneha-n left a comment

Choose a reason for hiding this comment

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

@delemaf congratulations on your first PR in zebra, looks great! minor changes/queries suggested.

}

export const BasicTable: React.FC<BasicTableProps> = React.memo(
({ columns, rows, onChange = () => {}, showRowIndex = false }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have a default value for onChange(), or should we check if(onChange()) before every call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure every table will need onChange so I left default value but I don't know what's best

src/webapp/components/table/BasicTable.tsx Outdated Show resolved Hide resolved
src/webapp/components/table/BasicTable.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@anagperal anagperal left a comment

Choose a reason for hiding this comment

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

thanks @delemaf ! It looks great this new component 🎉 together with Sneha's comments I've left some others

@delemaf delemaf requested review from anagperal and 9sneha-n July 2, 2024 08:38
Base automatically changed from feature/create-main-components-and-layout to development July 8, 2024 09:45
Copy link
Contributor

@anagperal anagperal left a comment

Choose a reason for hiding this comment

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

@delemaf everything looks good to me! 🚀 just add the useCallback and that's all for my point of view

@delemaf delemaf requested a review from anagperal July 10, 2024 08:04
@anagperal
Copy link
Contributor

@delemaf now the table looks like this:

image

it's ok?

@delemaf
Copy link
Contributor Author

delemaf commented Jul 10, 2024

Header background color wasn't right so I fixed it 👍
image
I'll remove the testing code also!

Copy link
Contributor

@anagperal anagperal left a comment

Choose a reason for hiding this comment

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

thansk @delemaf ! It looks good to me!

Copy link
Contributor

@9sneha-n 9sneha-n left a comment

Choose a reason for hiding this comment

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

looks great, thanks @delemaf

@bhavananarayanan bhavananarayanan merged commit 9eed5bd into development Jul 29, 2024
1 check passed
@bhavananarayanan bhavananarayanan deleted the feature/create-basic-table-component branch July 29, 2024 07:24
@9sneha-n 9sneha-n mentioned this pull request Oct 31, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants