Skip to content

set default value for empty integer and float fields - #1931

Merged
zubair-arbi merged 1 commit into
openedx:masterfrom
zubair-arbi:zub/bugfix/std894-deletingfloatfield
Dec 17, 2013
Merged

set default value for empty integer and float fields#1931
zubair-arbi merged 1 commit into
openedx:masterfrom
zubair-arbi:zub/bugfix/std894-deletingfloatfield

Conversation

@zubair-arbi

Copy link
Copy Markdown
Contributor

STUD-894

@cahrens @polesye
When deleting float/integer field values in Studio, set Model value to default value of that field.

Comment thread cms/static/js/views/metadata.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Why do we need (this.isIntegerField() || this.isFloatField()) here?
  2. What if this.min is undefined? In this case, integer/float fields will be empty.
  3. You can use value.length instead this.getValueFromEditor().length ( See line 197 ).
  4. Probably, will be better to use _.isNumber(this.min) instead this.min != undefined.

@cahrens

cahrens commented Dec 12, 2013

Copy link
Copy Markdown

I'm wondering what the behavior should be when the entry is deleted. I think it should actually revert to the default value for the field (as if you pressed the revert button next to the field), instead of the minimum value. @marcotuts what do you think?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of copying this code, pull the helper function verifyValueAfterChanged out and use it in both test methods.

@cahrens

cahrens commented Dec 16, 2013

Copy link
Copy Markdown

👍 After refactoring test code, removing the redundant conditions in Metadata.Number, and getting the thumbs-up from @marcotuts

Comment thread cms/static/js/views/metadata.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This view is used just for fields with type Integer and Float. Use else instead else if (this.isIntegerField() || this.isFloatField()). Also, isFloatField is redundant and can be removed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, thank you Anton. From the diff, I didn't realize the code was inside Metadata.Number.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You're welcome, Christina. I'm Anton :).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So sorry! Thank you, Anton. :)

@marcotuts

Copy link
Copy Markdown
Contributor

@zubair-arbi my suggestion here would be to treat a keyboard key deleting of text as a reset to the default value instead of using the min value (as cahrens described) This will keep keyboard deletes in sync with the revert button, which I think is preferred and would cause less confusion than using min and presuming that as a user override.

@polesye

polesye commented Dec 17, 2013

Copy link
Copy Markdown
Contributor

lgtm 👍

@cahrens

cahrens commented Dec 17, 2013

Copy link
Copy Markdown

👍

zubair-arbi added a commit that referenced this pull request Dec 17, 2013
…loatfield

set default value for empty integer and float fields
@zubair-arbi
zubair-arbi merged commit 9c8e90a into openedx:master Dec 17, 2013
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