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(Table): add component, theme, and story #9932

Merged
merged 25 commits into from
Aug 7, 2023

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Apr 8, 2023

Description

This is part of the Implementation of the DS v1

Creates a custom Table component, along with theming and a Storybook story file

Closes #8030

@gatsby-cloud
Copy link

gatsby-cloud bot commented Apr 8, 2023

✅ ethereum-org-website-dev deploy preview ready

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip I've based the creation of this new Table component on the current MarkdownTable, where only the table element is passed to the MDX provider and all the table elements are injected inside.

Based on the fact that Chakra's Table component is a multi-part component, I had to nest the styles for all the table elements inside the table part. And I am only using TableContainer and Table from Chakra, and not the child components.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip I've based the creation of this new Table component on the current MarkdownTable, where only the table element is passed to the MDX provider and all the table elements are injected inside.

Based on the fact that Chakra's Table component is a multi-part component, I had to nest the styles for all the table elements inside the table part. And I am only using TableContainer and Table from Chakra, and not the child components.

I'm going to see what needs to be done to use all of Chakra's table components to pass in as a group to the components object for markdown content. But I would do that in a separate PR as an improvement.

@pettinarip
Copy link
Member

@pettinarip I've based the creation of this new Table component on the current MarkdownTable, where only the table element is passed to the MDX provider and all the table elements are injected inside.
Based on the fact that Chakra's Table component is a multi-part component, I had to nest the styles for all the table elements inside the table part. And I am only using TableContainer and Table from Chakra, and not the child components.

I'm going to see what needs to be done to use all of Chakra's table components to pass in as a group to the components object for markdown content. But I would do that in a separate PR as an improvement.

Nice, sounds good.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip figured out that it was too easy to apply the Chakra table components, and ended up updating this PR with them implemented! 😄

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip I've updated the PR to integrate the new Table component to the templates.

I would point an immediate oddball, like on the /developers/docs/evm/opcodes/ page, where the table is too long in the space without overflow, compressing the header text. I would think resolving this by either extending the content width of the parent container, or allow overflow scrolling of the table with a higher-level min-width. Not sure how consistent this would be between templates. Let me know if you need any clarification here.

(Hopefully more consistency in the new DS with layouts will be helpful here!)

@pettinarip
Copy link
Member

Nice @TylerAPfledderer

I would point an immediate oddball, like on the /developers/docs/evm/opcodes/ page, where the table is too long in the space without overflow, compressing the header text. I would think resolving this by either extending the content width of the parent container, or allow overflow scrolling of the table with a higher-level min-width. Not sure how consistent this would be between templates. Let me know if you need any clarification here.

I think we would want to keep doing what we have in prod which is a horizontal scroll. cc @nloureiro what do you think of this case?

PR opcodes page

Copy link
Member

@pettinarip pettinarip 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 @TylerAPfledderer

} from "@chakra-ui/react"
import { MDXProviderComponentsProp } from "@mdx-js/react"

interface TableProps extends ThemingProps<"Table"> {
Copy link
Member

Choose a reason for hiding this comment

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

Curious, would it be the same as importing TableProps from the chakra package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct! .... kinda 😅

It only allows for the theming props and child components, and that's it. I was thinking to lock down the props, unless there was already a consideration for an instance or two that needed to apply other style props directly.

@nloureiro
Copy link
Contributor

The table looks great, awesome job! Just one detail we discussed before: the alignment on the head should be to the bottom of the cell and not to the top.
Screen Shot 2023-06-29 09 34 31 AM

@TylerAPfledderer
Copy link
Contributor Author

The table looks great, awesome job! Just one detail we discussed before: the alignment on the head should be to the bottom of the cell and not to the top. Screen Shot 2023-06-29 09 34 31 AM

@nloureiro This has now been added! 😄

@TylerAPfledderer
Copy link
Contributor Author

Nice @TylerAPfledderer

I would point an immediate oddball, like on the /developers/docs/evm/opcodes/ page, where the table is too long in the space without overflow, compressing the header text. I would think resolving this by either extending the content width of the parent container, or allow overflow scrolling of the table with a higher-level min-width. Not sure how consistent this would be between templates. Let me know if you need any clarification here.

I think we would want to keep doing what we have in prod which is a horizontal scroll. cc @nloureiro what do you think of this case?

PR opcodes page

Quick solution here is to remove the layout="fixed" prop, which is a prop that renders table-layout: 'fixed'. But for this opcodes page in particular, it would result in this:
image

Which of course makes it look like there is no horizontal overflow. (There is a lot of content farther down in the Initial Stack column)

A downside to removing the layout="fixed" is other tables with fewer columns not being able to properly expand the width of their respective pages. (See Staking Withdrawls)

@pettinarip
Copy link
Member

@TylerAPfledderer

Agreed. I think we need to remove the fixed layout only in the opcodes page for now (unless we detect another case). That will show it in a similar way as we have in prod.

Remove the Table `layout` prop that was set to fixed so it can go back to the default of auto and allow cells to expand width based on content.
@TylerAPfledderer
Copy link
Contributor Author

@pettinarip pushed changes. Should be good to go?

@pettinarip
Copy link
Member

@TylerAPfledderer same as in the Link PR, it looks good to me but since this PR touches many parts of the website, I want more eyes on it.

It'd be also nice if @nloureiro could take another look at this once he is back.

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

Did some spot checking on the tables noted above... they're looking great! Seeing proper x-scroll where needed, header appear aligned appropriately (bottom-left by default), custom alignments (ie: | ---: |) appear to be working correctly, and the cells are also matching that horizontal alignment, and seem to be aligned to the top as requested.

Thanks for all the efforts on this @TylerAPfledderer!

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

🚢

@pettinarip pettinarip merged commit a641422 into ethereum:dev Aug 7, 2023
@TylerAPfledderer TylerAPfledderer deleted the feat/new-table-theme branch August 7, 2023 15:27
This was referenced Aug 9, 2023
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.

Visual regression in markdown tables
4 participants