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

Changing Post/Survey Title Description fields to be editable #415

Merged
merged 3 commits into from
Nov 15, 2016

Conversation

willdoran
Copy link
Contributor

@willdoran willdoran commented Nov 10, 2016

This pull request makes the following changes:

  • Allow editing of Survey Title Description field labels

Test these changes by:

  • Create a new survey
  • Edit the Title and Description field labels
  • Ensure it saves
  • Create a new post based on this survey and ensure the field labels appear as set
  • Edit an existing survey
  • Edit the Title and Description field labels
  • Ensure it saves
  • Edit an existing post based on this survey and ensure the field labels appear as set
  • Create a new post based on this survey and ensure the field labels appear as set

Fixes ushahidi/platform#1469

Ping @ushahidi/platform


This change is Reviewable

@rjmackay rjmackay temporarily deployed to ushahidi-platform-devel-pr-415 November 10, 2016 03:07 Inactive
@coveralls
Copy link

coveralls commented Nov 10, 2016

Coverage Status

Coverage increased (+0.1%) to 52.469% when pulling 87559e0 on 1469-change-default-fields into ec638b2 on develop.

@ushbot ushbot temporarily deployed to rackspace_pr November 10, 2016 03:11 Inactive
Copy link
Contributor

@rjmackay rjmackay left a comment

Choose a reason for hiding this comment

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

Looks good so far. Few issues in comments.

@@ -113,11 +116,17 @@ function PostEditorController(
// If attributesToIgnore is set, remove those attributes from set of fields to display
var attributes = [];
_.each(attrs, function (attr) {
if (!_.contains($scope.attributesToIgnore, attr.key)) {
if (attr.type === 'title' || attr.type === 'description') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this first if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think probably not, I'm not sure what it was for

@@ -113,11 +116,17 @@ function PostEditorController(
// If attributesToIgnore is set, remove those attributes from set of fields to display
var attributes = [];
_.each(attrs, function (attr) {
if (!_.contains($scope.attributesToIgnore, attr.key)) {
if (attr.type === 'title' || attr.type === 'description') {
if (attr.type === 'title') {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be cleaner (though less efficient) to just do
_.findWhere(attrs, {type: 'title}) and then process that.. rather than adding many conditions to a loop

<label translate="survey.require_field">Require this field be completed</label>
<div class="toggle-switch">
<input class="tgl" id="switchnewattribute" type="checkbox" ng-model="editAttribute.required">
<label class="tgl-btn" for="switchnewattribute"></label>
</div>
</div>
<div class="form-sheet" ng-show="editAttribute.form_stage_id">
<div class="form-sheet" ng-show="editAttribute.form_stage_id" ng-show="editAttribute.type !== 'title' && editAttribute.type !== 'description'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple ng-show's won't work here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -3,7 +3,7 @@
<label translate="app.name">Name</label>
<input type="text" ng-model="editAttribute.label">
</div>
<div class="form-field" ng-if="editAttribute.input !== 'upload'">
<div class="form-field" ng-if="editAttribute.input !== 'upload' && editAttribute.type !== 'title' && editAttribute.type !== 'description'">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kinda ugly.. could we move this to a canSetDefault method on the scope?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually given the 3 new checks.. it seems like you could just wrap all the the form after label in 1 large ng-if and it'd be cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ushbot ushbot temporarily deployed to rackspace_pr November 11, 2016 23:14 Inactive
@willdoran willdoran dismissed rjmackay’s stale review November 15, 2016 16:26

Points addressed

@willdoran willdoran merged commit 57b33b9 into develop Nov 15, 2016
@sethburtonhall sethburtonhall deleted the 1469-change-default-fields branch February 16, 2017 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants