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

fix bug of subject admin page rewriting subject name as #249

Merged
merged 2 commits into from
Feb 14, 2017

Conversation

annyhe
Copy link
Contributor

@annyhe annyhe commented Feb 14, 2017

relatedLinks name

  • fix bug of inputs with identical name overriding each other
  • fix lint errors in nearby files

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling 0670a36 on fixSubjectAdmin into 05779c0 on master.

iamigo
iamigo previously requested changes Feb 14, 2017
@@ -154,14 +179,15 @@ function getFormData(form, aspectRangeFormat, propertyMetaData) {
if (aspectRangeFormat === 'BOOLEAN') {
fromBooleanInput(inputs, jsonData);
} else if (aspectRangeFormat === 'NUMERIC') {
fromTextInput(inputs, jsonData, function(value) {
fromTextInput(inputs, jsonData, (value) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

fromTextInput(inputs, jsonData, (value) => parseFloat(value));

return parseFloat(value);
});
} else if (aspectRangeFormat === 'PERCENT') {
fromTextInput(inputs, jsonData, function(value) {
return value/100;
fromTextInput(inputs, jsonData, (value) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

fromTextInput(inputs, jsonData, (value) => value / 100);

@@ -80,9 +85,19 @@ function fromTextInput(inputs, jsonData, alterDataFunc) {
* @returns {Object} with keys to particular DOM elements
*/
function getInputsAndSelects(form) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way to add a test for this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are ways but they are verbose to write and maintain. The crux of the method is node.contains, which is well-documented https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-249 February 14, 2017 18:36 Inactive
@annyhe
Copy link
Contributor Author

annyhe commented Feb 14, 2017

Demo app deployed to http://refocus-pr-249.herokuapp.com/subjects

@annyhe annyhe temporarily deployed to refocus-pr-249 February 14, 2017 19:07 Inactive
@annyhe annyhe dismissed iamigo’s stale review February 14, 2017 19:08

implemented most changes

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling beecc07 on fixSubjectAdmin into 05779c0 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.091% when pulling a7c5bee on fixSubjectAdmin into 05779c0 on master.

@annyhe annyhe merged commit e990e46 into master Feb 14, 2017
@annyhe annyhe deleted the fixSubjectAdmin branch February 14, 2017 22:06
@iamigo iamigo added the bug label Mar 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants