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

fix: dropzone id and event handling #433

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

oliverwebr
Copy link
Contributor

@oliverwebr oliverwebr commented Nov 18, 2022

Closes #432

πŸ“‘ Description

Dropzone is broken as hidden input has not have an id.
I added an id to the hidden input and in the second commit I added a id prop as the example page shows that Dropzone gets passed an id.

βœ… Checks

  • My pull request adheres to the code style of this project
  • I have updated the documentation as required (props gen)
  • All the tests have passed
  • My pull request is based on the latest commit (not the npm version).

@vercel
Copy link

vercel bot commented Nov 18, 2022

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
flowbite-svelte βœ… Ready (Inspect) Visit Preview Dec 6, 2022 at 3:22PM (UTC)
flowbite-svelte-update βœ… Ready (Inspect) Visit Preview Dec 6, 2022 at 3:22PM (UTC)

@shinokada
Copy link
Collaborator

Thank you for your PR.

Users may use more than one dropzone. This means we can't use the same id:

export let id: string = 'dropzone-file';

This can be avoided by:

let id:string ='' ;
id ||= 'dropzone-file'

So if users don't assign the id, dropzone-file will be used. If users assign the id, it will use their id.
What do you think?

@jjagielka
Copy link
Contributor

What about:

import generateId from '$lib/utils/generateId';
export let id: string = generateId();

@oliverwebr
Copy link
Contributor Author

let id:string ='' ;
id ||= 'dropzone-file'

So if users don't assign the id, dropzone-file will be used. If users assign the id, it will use their id. What do you think?

But is that not the same as my code? Users can set the id value 'dropzone-file' is just the default value.

What about:

import generateId from '$lib/utils/generateId';
export let id: string = generateId();

I think default values should be able to provide value for the lazy user. If they are generated they would be different between each build and could lead to confusion. What do you think?

@jjagielka
Copy link
Contributor

What will happen then if you have to drop zones on the page. With the generated id no issue, with the default value you'll end up with double id.

@oliverwebr
Copy link
Contributor Author

oliverwebr commented Nov 23, 2022

What will happen then if you have to drop zones on the page. With the generated id no issue, with the default value you'll end up with double id.

Sure but you can set the id. But even if you have just one dropzone on your page which is part of a form you are forced to set an id because otherwise, the file will appear always under a different payload property (if send in a form etc).

If you still prefer the generated approach let me know.

@oliverwebr oliverwebr changed the title WIP: Fix/dropzone id Fix/dropzone id Nov 23, 2022
@oliverwebr oliverwebr changed the title Fix/dropzone id Fix/dropzone id and event handling Nov 23, 2022
@oliverwebr oliverwebr changed the title Fix/dropzone id and event handling fix: dropzone id and event handling Nov 23, 2022
@jjagielka
Copy link
Contributor

There are lots of valuable input in that proposal. Few comments:

  • you don't need to wrap the label in button: tabIndex=0 on label will do the trick
  • input is embedded into the label so attr for is not needed
  • with the above $$restProps on input will solve the id discussion
  • handling click is not necessary as click on label causes a click on the input
  • handling of pressing Space and Enter is therefore needed as those should provoke click on input
  • input is of type file so should support binding to files property
  • I have just uploaded a trimmed down layout for drop zone
    So my proposal for that PR would be:
<script lang="ts">
  import classNames from 'classnames';

  export let value: string = '';
  export let files: FileList | undefined = undefined;
  export let defaultClass: string =
    'flex flex-col justify-center items-center w-full h-64 bg-gray-50 rounded-lg border-2 border-gray-300 border-dashed cursor-pointer dark:hover:bg-bray-800 dark:bg-gray-700 hover:bg-gray-100 dark:border-gray-600 dark:hover:border-gray-500 dark:hover:bg-gray-600';

  let input: HTMLInputElement;

  function keydown(ev: KeyboardEvent) {
    if ([' ', 'Enter'].includes(ev.key)) {
      ev.preventDefault();
      input.click();
    }
  }
</script>

<label
  class={classNames(defaultClass, $$props.class)}
  tabIndex="0"
  on:keydown={keydown}
  on:focus
  on:blur
  on:mouseenter
  on:mouseleave
  on:mouseover
  on:dragenter
  on:dragleave
  on:dragover
  on:drop>
  <slot />
  <input {...$$restProps} bind:value bind:files bind:this={input} type="file" class="hidden" on:change on:click />
</label>

@oliverwebr
Copy link
Contributor Author

@jjagielka awesome, you are absolutely right I adopted your suggestion. πŸŽ‰

@jjagielka
Copy link
Contributor

@shinokada please approve that PR.

@shinokada shinokada merged commit 6e4d9d1 into themesberg:main Dec 7, 2022
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.

Dropzone input missing id
3 participants