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

Add class and style to table column #13

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

msaraiva
Copy link
Member

@msaraiva msaraiva commented Feb 6, 2021

This PR adds a style prop to <Column>. This way the user can customize the <th> style directly. I also updated surface_catalogue to the latest version and created a playground for the table.

@msaraiva
Copy link
Member Author

msaraiva commented Feb 6, 2021

It also can be used to set the column width directly until we have a solution for #10.

@msaraiva msaraiva changed the title Add style to table column Add class and style to table column Feb 7, 2021
@msaraiva
Copy link
Member Author

msaraiva commented Feb 7, 2021

Hey @olivermt!

I realized that for a more generic solution, it would be nice to also have a class property for <Column> so I updated the PR. I also added a couple of tests for them. I believe this way the user has full control on how to customize the <th>.

@olivermt
Copy link
Collaborator

olivermt commented Feb 7, 2021

@msaraiva I am thinking we should probably split this into header_class and item_class so we have an easy way to propagate styles down to each as well.

@olivermt
Copy link
Collaborator

olivermt commented Feb 7, 2021

Also this is just raw styles right? I am not completely sold on having that as an api in a css driven framework.
I am thinking we should probably expose prop width, :string, values: ["1", "2", ... "11", "12"] and translate that internally to flex layout which also follows the split-by-12 pattern in bulma.

I am thinking that we shouldnt expose a style prop until we're sure how we want to integrate flex layout to tables with the bigger bulma picture.

@olivermt
Copy link
Collaborator

olivermt commented Feb 7, 2021

I have a suggestion, what if we had a Style component that is collected via macros and is embeddable in as a scss combinator? Isn't that similar to what you were discussing in the issue over in Surface?

@msaraiva
Copy link
Member Author

msaraiva commented Feb 7, 2021

I am thinking we should probably split this into header_class and item_class so we have an easy way to propagate styles down to each as well.

But a header_class would apply to the header's <tr>, right? In this case, it will apply to the <th>

Also this is just raw styles right? I am not completely sold on having that as an api in a css driven framework.
I am thinking we should probably expose prop width, :string, values: ["1", "2", ... "11", "12"] and translate that internally to flex layout which also follows the split-by-12 pattern in bulma.

I think we should have both. It's really hard to predict all scenarios the user might need. Eventually, we'll need to make a choice and pick up a specific API as default (like the one you suggested) but I still think users may need to do extra customizations that we'll not be able to bring as standard API so exposing class and style for the main parts seems like a natural way to do that.

what if we had a Style component that is collected via macros and is embeddable in as a scss combinator? Isn't that similar to what you were discussing in the issue over in Surface?

I believe it's very similar.

@msaraiva
Copy link
Member Author

msaraiva commented Feb 7, 2021

Anyway, the decision is totally yours. If you have any doubt that this is the way to go, feel free to close the PR. It's your baby now 😉

You could, however, keep the playground and update the catalogue version. It's really cool to play with the component's properties and see how they affect the visual of the table 🙂

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.

2 participants