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 part #376: Fraction input interaction view validation #419

Merged
merged 107 commits into from
Jan 22, 2020

Conversation

nikitamarysolomanpvt
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt commented Nov 19, 2019

Explanation

Added validation for fraction input with error messages given below.
INVALID_CHARS = "Please only use numerical digits, spaces or forward slashes (/)"
INVALID_FORMAT = "Please enter a valid fraction (e.g., 5/3 or 1 2/3)"
DIVISION_BY_ZERO = "Please do not put 0 in the denominator"

For testing purpose make following change in code:

Launcher activity should be InputInteractionViewTestActivity

Design Doc

https://docs.google.com/document/d/1QigNdZeMBy-JjHW26QH4qT0UtCyzfvW0xEaStSdY9gM/edit#heading=h.2i0txgpugkgf

Reference

https://www.oppia.org/explore/rfX8jNkPnA-1?collection_id=4UgTQUc1tala

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

nikitamarysolomanpvt and others added 30 commits November 11, 2019 15:56
…://github.com/oppia/oppia-android into hi-fi-input-interaction-views

# Conflicts:
#	app/src/main/res/layout/text_input_interaction_item.xml
…://github.com/oppia/oppia-android into hi-fi-input-interaction-views

# Conflicts:
#	app/src/main/res/layout/text_input_interaction_item.xml
On click of input type interaction item, it requires two clicks to display keyboard, which should actually be just a single click.
thus preventing scroll due to edit text focus
…ToPosition(0) in processEphemeralStateResult
@rt4914
Copy link
Contributor

rt4914 commented Jan 17, 2020

@nikitamarysolomanpvt I have checked your last commits they look fine to me. I will need to defer to @veena14cs on this PR.

@rt4914 rt4914 assigned veena14cs and unassigned veena14cs and rt4914 Jan 17, 2020
@mschanteltc
Copy link

LGTM

Copy link
Contributor

@veena14cs veena14cs left a comment

Choose a reason for hiding this comment

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

LGTM

@veena14cs veena14cs removed their assignment Jan 20, 2020
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks! This looks fantastic. Sorry for the long review--once my comments are addressed feel free to submit this.

Also, did you reach out to someone on the Oppia web team to discuss implementing the new 7-digit error on there? (FYI @seanlip).

@nikitamarysolomanpvt
Copy link
Contributor Author

Thanks! This looks fantastic. Sorry for the long review--once my comments are addressed feel free to submit this.

Also, did you reach out to someone on the Oppia web team to discuss implementing the new 7-digit error on there? (FYI @seanlip).

@BenHenning PTAL oppia/oppia#8157 (comment)

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@rt4914 rt4914 removed their assignment Jan 22, 2020
Copy link
Contributor

@veena14cs veena14cs left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants