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: add selectUpload config option to ImageBlock #1559

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

geertplaisier
Copy link

@geertplaisier geertplaisier commented Nov 13, 2024

  • I read the contributing guide
  • I agree to follow the code of conduct

Summary

Currently it is possible to configure an onUpload method for the ImageBlock component. This allows you to upload the selected image and return the URL. For our application we already have a collection of images available and it would be nice if the user could also select an image instead of always uploading a new one.

For this I added an selectUpload method which can be configured for the ImageBlock component. This expects a Promise as return value, which works similar to the onUpload configuration option but does not pass a file. Hopefully this is useful for others as well.

How did you test this change?

Since the implementation for this can vary a lot for each implementation, I added a Storybook story to demonstrate the behavior.

Copy link

changeset-bot bot commented Nov 13, 2024

⚠️ No Changeset found

Latest commit: c22666b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Nov 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
milkdown-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 13, 2024 3:40pm

@Saul-Mirone
Copy link
Member

This is a little confusing to me. How does the users know what url they should return in the promise if there's no additional parameters?

@geertplaisier
Copy link
Author

@Saul-Mirone The only difference between the existing onUpload option and the new selectUpload is that in the first case the end-user gets a system file picker to select a file from the file system and with the latter you would need to implement some kind of file picker yourself. This could be used for example in a CMS where there is an existing library of images to choose from. Hope this makes sense, if not I can extend the storybook example.

@@ -142,7 +157,7 @@ export const imageComponent: Component<ImageComponentProps> = ({
/>
<div class=${clsx('placeholder', hidePlaceholder && 'hidden')}>
<input disabled=${readonly} class="hidden" id=${uuid} type="file" accept="image/*" onchange=${onUpload} />
<label onpointerdown=${onClickUploader} class="uploader" for=${uuid}>
<label onpointerdown=${onClickUploader} onclick=${handleUpload} class="uploader" for=${uuid}>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add a new event handler here to handle both onclick and onpointerdown, a possible solution should be still use onUpload without introducting another config. We can pass event to the onUpload config to let users open their own file picker.

Copy link
Member

@Saul-Mirone Saul-Mirone left a comment

Choose a reason for hiding this comment

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

We should also consider the image-inline component in crepe.

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