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

Implemented option to append to an exisiting file #65

Merged
merged 5 commits into from
Jul 6, 2015

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Jun 24, 2015

This is a PR for #56.
@qzminski and @aschempp, please review.

@Toflar Toflar force-pushed the feature/append-to-file branch from d6767b3 to 39d9f13 Compare June 24, 2015 23:23
@Toflar Toflar added this to the 1.3.0 milestone Jun 24, 2015
@Toflar Toflar mentioned this pull request Jun 24, 2015
@qzminski
Copy link
Member

Looks good

@aschempp
Copy link
Member

Can you explain why you changed the checkbox to a dropdown? What are the three different states? Create a new file, overwrite, or append? A blank option seems allowed, but I don't know what it would do?

@Toflar
Copy link
Member Author

Toflar commented Jun 25, 2015

A blank option seems allowed, but I don't know what it would do?

It would neither override, nor append = create a new file every time you submit the notification :)

@aschempp
Copy link
Member

That's not really clear with the selection, is it? Maybe we should add a blank option label?

@Toflar
Copy link
Member Author

Toflar commented Jun 25, 2015

Well it wasn't there until now either. I could have added another checkbox to define if it should append but then you can check not check "override" and check "append" which totally makes no sense^^ But yeah, we could provide a blank option label.

@aschempp
Copy link
Member

Either that or include a third option and make the field mandatory (which would be better with the class constants). The current implementation is taken from the Contao file upload field, but that does not support appending so we need to find something new.

@Toflar
Copy link
Member Author

Toflar commented Jul 6, 2015

I have updated the PR :)

@aschempp
Copy link
Member

aschempp commented Jul 6, 2015

👍

Toflar added a commit that referenced this pull request Jul 6, 2015
Implemented option to append to an existing file
@Toflar Toflar merged commit b3cf548 into develop Jul 6, 2015
@Toflar Toflar deleted the feature/append-to-file branch July 6, 2015 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants