-
Notifications
You must be signed in to change notification settings - Fork 2k
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
@uppy/provider-view: allow apps to render custom forms in AuthView #4555
Conversation
a5c9bf1
to
9f3321d
Compare
{inputs?.map((i) => ( | ||
<div className="uppy-Provider-authInput"> | ||
<label htmlFor={`uppy-Provider-authInput-${i.id}`}> | ||
<span>{i.label}</span> | ||
<input id={`uppy-Provider-authInput-${i.id}`} name={i.name} type={i.type || 'text'} defaultValue={i.defaultValue} /> | ||
</label> | ||
{i.description && (<span>{i.description}</span>)} | ||
</div> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is too tightly coupled. We probably want some sort of inversion of control here, like is done for metaFields
in Dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is too tightly coupled to what?
I'm not sure what you would like here and what for.
I see, it might indeed be useful to have a custom render function apart from that I would need some examples of what you imagine.
Feel free to push anything if that makes it easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inputs
is tightly coupled to a very specific HTML structure. If someone wants to change even one small thing, it's not possible. Therefor I think it's better to keep something like this flexible with inversion of control: exposing a a function which returns preact elements.
Example from Dashboard:
uppy.use(Dashboard, {
trigger: '#pick-files',
metaFields: [
{ id: 'name', name: 'Name', placeholder: 'file name' },
{ id: 'license', name: 'License', placeholder: 'specify license' },
{
id: 'caption',
name: 'Caption',
placeholder: 'describe what the image is about',
},
{
id: 'public',
name: 'Public',
render({ value, onChange, required, form }, h) {
return h('input', {
type: 'checkbox',
required,
form,
onChange: (ev) => onChange(ev.target.checked ? 'on' : ''),
defaultChecked: value === 'on',
});
},
},
],
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, makes sense! I'll try to implement that asap :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think I should reuse RenderMetaFields
for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my poc experiment i'm doing like this:
this.view = new ProviderViews(this, {
provider: this.provider,
viewType: 'list',
showTitles: true,
showFilter: true,
showBreadcrumbs: true,
renderAuthForm: () => (
<div className="uppy-Provider-authInput">
<label htmlFor="uppy-Provider-publicLinkURL">
<span>{this.i18n('publicLinkURLLabel')}</span>
<input id="uppy-Provider-publicLinkURL" name="webdavUrl" type="text" />
</label>
<span>{this.i18n('publicLinkURLDescription')}</span>
<button style={{ display: 'block' }} type="submit">Submit</button>
</div>
),
})
not sure if it's the best way but render props are very flexible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, renderMetaFields provides a standard implementation if you don't care too much but allows you to specify custom render method.
This (your implementation) would work for us obviously
I think this can be superseded by #4619 |
see #4551 for context, it's a prerequisite for this PR - without #4551 setting
customQueryParams
has no effect.This probably still needs some styling love and translation support, but I would like to get a first round of feedback on this approach before I invest more time into this.