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

[#304] Added warning message for updates without text, photo and video #465

Merged
merged 3 commits into from
Mar 26, 2014

Conversation

KasperBrandt
Copy link
Contributor

@zzgvh can you code check?

@zzgvh
Copy link
Contributor

zzgvh commented Mar 21, 2014

Yep, perfect functionality.

However the world is not as simple as one would wish...

Number one, and this is a constant annoyance for us all, but we'll have to live with it for the time being: there are two sets of templates, one for RSR and one for Partner sites aka Pages. So the functionality needs to be added to the partner site template too 😒 (templates/partner_sites/project/update_form.html). This leads to the question if the script should be made into a separate file, or rather added to an existing one. But in this case I think not, because of:

Number two, the alert box text should be marked for translation. And since we don't run javascript files through the translation machinery it's probably best to have the script inline in the template even if it isn't DRY. Alternatively the JS is split up into two parts; one script file and one inline assignment of the text to a variable. If you do it this way, talk to Daniel about how to get a new .js into our custom asset manager. I actually don't remember.

And a couple of style comments (which are of course at least somewhat personal 😉).

The variable assignments can be done in one go like so:

var text_val=document.forms["add_update_form"]["text"].value,
    photo_val=document.forms["add_update_form"]["photo"].value,
    video_val=document.forms["add_update_form"]["video"].value;

Regardless statements should have an ";" at the end even tho it's not strictly needed (see discussion in Akvo dev chat from yesterday!).

Last, single character variable names should be limited to for loop counters and the like. Even if it's only used once at the next line as is the case with r in this case. Also I think that if (r==true) can be replaced with if (r) since I'm guessing that confirm() returns a boolean.

@KasperBrandt
Copy link
Contributor Author

@zzgvh Added the translations just as variables and fixed all your comments. Could you check again?

@zzgvh
Copy link
Contributor

zzgvh commented Mar 26, 2014

Code reviewed, all systems green.

zzgvh added a commit that referenced this pull request Mar 26, 2014
[#304] Added warning message for updates without text, photo and video
@zzgvh zzgvh merged commit e7748de into develop Mar 26, 2014
@zzgvh zzgvh deleted the kasper-304 branch March 26, 2014 11:49
@adriancollier adriancollier removed this from the 2.3.5 Uglyfruit milestone Apr 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants