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

Add attachments to proposals #1688

Merged
merged 10 commits into from
Aug 10, 2017
Merged

Conversation

beagleknight
Copy link
Contributor

@beagleknight beagleknight commented Aug 4, 2017

🎩 What? Why?

This adds attachments to proposals both for official and not official ones. As a user I will be able to upload a single attachment to a proposal. Uploads can be images or pdfs.

📌 Related Issues

📋 Subtasks

  • Feature spec
  • User can upload attachments when creating a proposal
  • Admin can upload attachments when creating a proposal
  • Update CHANGELOG
  • Remove checkbox and add text instead

📷 Screenshots (optional)

image
image

👻 GIF

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #1688 into master will decrease coverage by 0.01%.
The diff coverage is 96.45%.

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
- Coverage   98.59%   98.58%   -0.02%     
==========================================
  Files         966      968       +2     
  Lines       22029    22163     +134     
==========================================
+ Hits        21720    21849     +129     
- Misses        309      314       +5

Copy link
Contributor

@josepjaume josepjaume left a comment

Choose a reason for hiding this comment

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

Should we maybe put uploads under a global setting flag? It might be surprising for existing processes to suddenly see uploads enabled.

@josepjaume
Copy link
Contributor

josepjaume commented Aug 7, 2017

Why do we have "attachment terms"?

@josepjaume
Copy link
Contributor

(And where do we customize that?)

@josepjaume
Copy link
Contributor

Also, the catalan translation of "crear proposta" seems to be wrong, it should be "autor de la proposta", but that's outside of the scope of this PR.

@mention-bot
Copy link

@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @oriolgual and @divins to be potential reviewers.

@beagleknight
Copy link
Contributor Author

@josepjaume It is already a global setting 😅

@beagleknight
Copy link
Contributor Author

beagleknight commented Aug 7, 2017

@josepjaume argh.. I can't answer your comments individually...

Why do we have "attachment terms"?

@oriolgual suggested this checkbox to warn the users to upload safe content, etc. I am not sure what copy may I use 😄

(And where do we customize that?)

It could be a setting as well... but then we need i18n for feature settings (I am already working on it in another branch, not published yet).

Also, the catalan translation of "crear proposta" seems to be wrong, it should be "autor de la proposta", but that's outside of the scope of this PR.

Yeah, I saw it 😄 . We need to do a revision of public forms copys.

@josepjaume
Copy link
Contributor

@beagleknight as for the attachment terms, I would just put an explanation there that's hard-coded into the app. No need to customize that nor to have a checkbox.

Thoughts @oriolgual?

@oriolgual
Copy link
Contributor

Seems OK for me, the problem is that I'm not sure what those terms should be :/ @josepjaume @beagleknight

@beagleknight
Copy link
Contributor Author

@josepjaume @oriolgual I can remove the checkbox and add the text you mentioned as soon as we have it 😄

@beagleknight beagleknight force-pushed the feature/proposal-attachments branch 2 times, most recently from ad1e8d6 to 3876277 Compare August 9, 2017 07:37
josepjaume
josepjaume previously approved these changes Aug 9, 2017
@josepjaume
Copy link
Contributor

I would merge this and add the text on a further PR @beagleknight . Thoughts @oriolgual ?

@josepjaume
Copy link
Contributor

We need to add hints and support text everywhere sometime soon anyway.

@beagleknight
Copy link
Contributor Author

@josepjaume I agree but... should we remove the checkbox then?

@josepjaume
Copy link
Contributor

@beagleknight yeah, I would remove it 👍

@beagleknight beagleknight merged commit 1504db8 into master Aug 10, 2017
@beagleknight beagleknight deleted the feature/proposal-attachments branch August 10, 2017 10:05
@ghost ghost removed the in-review label Aug 10, 2017
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.

5 participants