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

Inline-Editing: allow client to provide custom components #33

Open
wants to merge 6 commits into
base: playground
Choose a base branch
from

Conversation

mavcook
Copy link
Contributor

@mavcook mavcook commented Aug 31, 2023

I took a stab at resolving #29, which allows users to use a custom component when editing a cell in the table. My approach was to create an object for edit related configuration in the Instructs interface. This gives a user almost full control over how they want editing to work, and leaves room for more capabilities and tuning in the future.

Check the updated example7/ code for full context, but below gives a quick idea of the end result.

let ptInstructs: Instructs[] = [
    {
        key: 'first_name',
    },
    {
        title: 'Favorite Color',
        key: 'favoriteColor', 
        edit: {
            as: <ComponentType<SvelteComponent>>MyDropdownComponent,
            props: {selectValues: ['Red', 'Blue', 'Green']}
        },
    },
]

We add a custom component for the favoriteColor column that is a dropdown menu
image

Changes

  • EditBlock component encapsulates the default textarea edit behavior
  • Submitting edits now occurs through a Svelte Component event

Client/User Breakage

  • B1. Submitted edits no longer dispatch a rowClicked event
  • B2. Clients using dataComponents will get warning <component> was created with unknown prop 'instructs' (this can be easily reverted)

Notes

N0. This is a larger change than initially anticipated, and as such it introduces slightly different behavior. If the change are too much, or not the right approach, I am happy to rework anything.

N1. Adding the Cell interface made documentation and user adoption of the edit component a bit easier, but maybe isn't the right naming or approach. I didn't see any way to inhertance or templating in Svelte


fixes #29

@muonw-public
Copy link

Thank you! It's a great implementation. Since you have two commits that somewhat overlap, it might be easier to see the detailed suggestions here in this temporary branch (some are just for suggesting an alternative name and a few are changes to the parts that where not part of your commits):

724ef81

The only major change is the removal of the Cell interface even though it makes the code cleaner (you will find a comment on the reason for this).

app/src/lib/components/PowerTable.svelte Outdated Show resolved Hide resolved
app/src/lib/components/PowerTable.svelte Outdated Show resolved Hide resolved
app/src/lib/components/PowerTable.svelte Outdated Show resolved Hide resolved
app/src/lib/components/PowerTable.svelte Outdated Show resolved Hide resolved
app/src/lib/components/PowerTable.svelte Outdated Show resolved Hide resolved
app/src/lib/styles/power-table-mascara.scss Outdated Show resolved Hide resolved
app/src/routes/examples/example7/MyComponent.svelte Outdated Show resolved Hide resolved
app/src/routes/examples/example7/MyComponent.svelte Outdated Show resolved Hide resolved
app/src/routes/examples/example7/+page.svelte Outdated Show resolved Hide resolved
@mavcook
Copy link
Contributor Author

mavcook commented Sep 3, 2023

Nice, thank you for the review and changes! It is cool learning more about javascript and Svelte performance.

It looks like you made a lot of the changes in your suggestions commit, so I resolved the related conversations. I will pull your commit into my branch, and I will work on the remaining changes.

@mavcook
Copy link
Contributor Author

mavcook commented Nov 10, 2023

Hi @muonw-public, all previous feedback has been addressed, so this PR should be good to land when you get a chance to do a final check. Let me know if there is anything else you would like to see, or if I can help in any way. Thanks!

@muonw-public
Copy link

It's unclear how to preserve the integrity of data with this change. In particular, if we have props: {selectValues: ['Red', 'Blue', 'Green']} in the instructs, but data already contains the value 'Yellow', how should that be handled during the edit.
Regardless, it makes sense to continue waiting for the release of Svelte 5 as it may call for a rewrite.

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.

3 participants