-
-
Notifications
You must be signed in to change notification settings - Fork 302
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
Add a flag to enable image uploads #750
Conversation
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.
Interesting approach and neat use case for miniserve! Do you think there's value in actually verifying that a valid image has been uploaded? You can still upload anything you want even if it's not an image and nothing will actually check that.
@@ -104,6 +104,10 @@ pub struct CliArgs { | |||
#[clap(short = 'u', long = "upload-files")] | |||
pub file_upload: bool, | |||
|
|||
/// Enable image uploading | |||
#[clap(short = 'I', long = "upload-images")] |
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.
Should this flag also imply file_upload
or should it be exclusive to that flag?
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.
Oh, can you actually specify something like that in clap? I've never used it before, so I solved this implication in config.rs
. Come to think of it, this approach would theoretically allow to create inconsistent MiniserveConfig
values, where image_upload
is true, but file_upload
is false. Maybe we should combine both of these values into an enum or just always specify the accept
attribute and default it to *
. Then we could have a CLI argument to configure any accept
value you want, thus making this feature more 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.
Yes, check for instance this conflicts_with
or this requires
.
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.
That's neat, thanks for letting me know. I always used argh so far because of the lower compile time impact, maybe I should check out clap.
What do you think about the idea to pass the value for the accept
attribute instead of making this a flag? Any preference on your side?
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 that'd be kinda neat and a bit more elegant but potentially also a bit more annoying for users. If we do go that way, we need to make sure to add proper documentation to make users aware of the possibilities and implications.
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 that's true, we'd almost have to provide a table of potentially useful values and their effects if we choose this approach. But I think it may be worth it, maybe you can get a smartphone to open a video recorder or audio recorder with the right values.
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.
Can you try your theory? In that case we should perhaps enable it.
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.
Just gave it a try and ran the examples given on w3schools (https://www.w3schools.com/tags/att_input_accept.asp):
video/*
does indeed open the camera in video mode instead of stills modeaudio/*
opens the audio recorder app
So I guess there would be a benefit in allowing users to set these values as well.
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 I think that's pretty cool. Though I wonder how to best expose this and show it to the user especially considering this is most useful on mobile. I suppose perhaps we'd want to actually show multiple buttons on mobile potentially each with their own media specialization? Not quite sure to be honest, the UI might get too crowded. What are your thoughts on 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.
I think we don't need multiple buttons, you can actually just combine the individiual values. When browsing the site on Firefox on Android, a value like image/*,audio/*
lets me choose between the audio recorder and camera.
I think that leaves us with two useful options:
- Allow users to specify the value of the accept attribute directly in its raw form
- Provide an argument that collects values of a predefined
enum
into a list
Of course, we could also combine both approaches and have option 2 for ease of use and still allow option 1 for special use cases.
Yeah, miniserve allows to create small utilities like this really easily, thanks for creating it! I almost started to create my own web app for this before I found out about miniserve, this saved me a lot of time. For my use case there would not be any real benefit of validating the contents - I'm the only user here and don't intend to make this publicly accessible. |
Glad to hear!
I'd agree, just wanted to hear your thoughts. How do you think upload and image upload should interact? |
This implements the feature I asked about in #546.
An additional flag allows to add the
accept="image/*"
attribute to the file input.