illright / attractions

A pretty cool UI kit for Svelte
https://illright.github.io/attractions
MIT License
1.03k stars 37 forks source link

Support custom body in tables #348

Closed mijorus closed 1 year ago

mijorus commented 2 years ago

Hi, I have been using attractions this week but I quickly found that tables are pretty limited because by default you can't fully customize rows etc.

A quick fix might be this one, please note this is just POC code that I will improve further I you like the idea.

Basically tables would have a <slot /> named body where we can inject a <tbody /> element formatted as we like. Let me know what you think

aabounegm commented 2 years ago

The idea itself is valid, but the implementation can be improved. To begin with, I don't like the fact that a prop controls the usage of a slot and would prefer to have the tbody content be the fallback slot content. In other words, instead of

{#if customBody}
  <slot name="body" />
{:else}
  <tbody>...</tbody>
{/if}

it would be better if it looked like this:

<slot name="body">
  <tbody>...</tbody>
</slot>

without using a prop at all.

That also makes me think that it might be a good idea to add more slots for head and row for more granular control.

mijorus commented 2 years ago

Do you think the slot should be wrapped inside a tbody already or that it would be unnatural for a user to generate a table without wrapping it ?

mijorus commented 2 years ago

Do you know how I can test this package locally? Every time I try to npm install git://github.com/mijorus/attractions.git#custom-table-rows --save-dev it errors out:

npm ERR! git dep preparation failed
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm ERR! code EUNSUPPORTEDPROTOCOL
npm ERR! npm ERR! Unsupported URL Type "workspace:": workspace:^3.7.0
aabounegm commented 2 years ago

Do you think the slot should be wrapped inside a tbody already or that it would be unnatural for a user to generate a table without wrapping it?

I think it would make more sense to have it outside it so that usage would like as such:

<Table>
  <tbody slot="body">...</tbody>
</Table>

Do you know how I can test this package locally?

You can clone the repo locally (or open in GitHub Codespaces) and run pnpm docs:dev. You couldn't install it from git with npm because the package.json in the repo root is not the same as the one the package uses, we are using a monorepo style. You will need to use pnpm

illright commented 2 years ago

Alternatively, if you want to test out the changes in your own project, enter its directory and use pnpm link ./path/to/attractions-compiled/

mijorus commented 2 years ago

Please note this is still WIP. I need to find a way to tell Svelte to apply the SCSS style to a slot. My idea was to use the :globlal selector together with a special css class (like custom or attractions-table) applied to the table element to avoid conflicts with other custom table elements

mijorus commented 2 years ago

@illright I am sorry for the ugly solution, but it's the only one I have found

mijorus commented 2 years ago

I feel like the latest commit is a much better implementation both for the user and it's easier to maintain

aabounegm commented 2 years ago

I don't understand why you added <style lang="scss" src="../../../../node_modules/attractions/table/table.scss"></style> to the docs. The docs have their own styles and they should not affect the selectors of the original component. It is intentional that slots are to be styled by the containing component that fills them in, not the one in which they get inserted. Also, can you please make sure the CI passes and maybe add some documentation to the slots section of the page, not just the example?

mijorus commented 2 years ago

I want my table body to look just like a table body that you would create with the item prop, but the content of each box can be freely personalized (say I want a button as the last element of one of my rows)

In order to do this, I can define my custom tbody element, which however will now look completely different from the rest of the UI; to avoid this issue, I can import the attractions' stylesheet into my component, so that the table can look right.

aabounegm commented 2 years ago

I understand, but that is not the common use-case for slots; they are typically used with custom styles. You may import the scss file into your component, but we wouldn't want to modify the component's original styles, or the docs for that matter. For the example you described above, you might be better off using the item slot instead, and use the header and item slot props to identify which cell it is. I'm still not opposed to adding more slots, I just want them to be generic and not tailored for one specific usage scenario