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 #380: Hi fi input interaction views #405

Merged
merged 34 commits into from
Nov 19, 2019

Conversation

nikitamarysolomanpvt
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt commented Nov 18, 2019

Explanation

Whenever state is changed the scroll view should start from the top everytime.For this removed the focus from edittext.

Checklist

  • Whenever state is changed the scroll view should start from the top everytime.
  • On click of input type interaction item, it requires two clicks to display keyboard, which should actually be just a single click.

Reference Mocks

https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/4d4e4bd8-3755-41fc-bf83-3dcff5d5eee1/Lesson-1-a-ii-Exploration-Player-Text-Input-No-Ans
https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/eeed0f6c-54ca-4371-ab23-5199abd87f28/Lesson-1-a-ii-Exploration-Player-Text-Input-Keyboa
https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/d66097bf-8421-41d8-bfda-2450d6e98dea/Lesson-1-a-i-Exploration-Player-Fraction-Input-No-
https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/249c3349-605b-4569-bbe9-1a017ab90101/Lesson-1-a-i-Exploration-Player-Fraction-Input-No-
https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/c4b0dfb3-53b4-491e-a759-818e096183fd/Lesson-1-a-v-Exploration-Player-Numeric-Input-No-A

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.

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.

Currently, there is one case in which the new state does not start from the top-position:

add this line
binding.stateRecyclerView.smoothScrollToPosition(0)

at the end of StateFragmentPresenter -> processEphemeralStateResult function.

@nikitamarysolomanpvt
Copy link
Contributor Author

Currently, there is one case in which the new state does not start from the top-position:

add this line
binding.stateRecyclerView.smoothScrollToPosition(0)

at the end of StateFragmentPresenter -> processEphemeralStateResult function.

I fixed input view focus issue because of which the scroll was happening.
as per your suggestion added binding.stateRecyclerView.smoothScrollToPosition(0) in processEphemeralStateResult

…ToPosition(0) in processEphemeralStateResult
@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 18, 2019
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

@BenHenning BenHenning self-assigned this Nov 18, 2019
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.

Generally LGTM--thanks @nikitamarysolomanpvt! I had some comments that should definitely be addressed, but the solution seems sensible based on my understanding of focus behavior on Android. Also, I tried it out locally and it seems to work quite well.

@BenHenning BenHenning removed their assignment Nov 19, 2019
@mschanteltc
Copy link

@mschanteltc as per your reply in mail Fix #28,#29,#33: Hi-fi input interaction views #330

I agree that when the input is in focus, the keyboard would appear. However if the user taps on the screen that is not the input bar or the keyboard, the keyboard should collapse. If the input is still empty, then it would still show placeholder text. Otherwise if the input has characters in it, those characters remain in the bar and the placeholder text is not seen.

Please find the your suggestions implemented in this PR.

Thanks Nikita! Is it possible you can attach an image/gif so I can see how this works please?

@rt4914
Copy link
Contributor

rt4914 commented Nov 19, 2019

@nikitamarysolomanpvt This PR looks good to me including Ben's suggestions. Just reply to this comment and Chantel's 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.

@nikitamarysolomanpvt
Copy link
Contributor Author

nikitamarysolomanpvt commented Nov 19, 2019

@mschanteltc

@mschanteltc as per your reply in mail Fix #28,#29,#33: Hi-fi input interaction views #330

I agree that when the input is in focus, the keyboard would appear. However if the user taps on the screen that is not the input bar or the keyboard, the keyboard should collapse. If the input is still empty, then it would still show placeholder text. Otherwise if the input has characters in it, those characters remain in the bar and the placeholder text is not seen.

Please find the your suggestions implemented in this PR.

Thanks Nikita! Is it possible you can attach an image/gif so I can see how this works please?

gif

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 19, 2019
@nikitamarysolomanpvt
Copy link
Contributor Author

@nikitamarysolomanpvt This PR looks good to me including Ben's suggestions. Just reply to this comment and Chantel's comment.

Done

@nikitamarysolomanpvt nikitamarysolomanpvt merged commit 7704741 into develop Nov 19, 2019
@mschanteltc
Copy link

When the input is empty, can we make the "Submit" button disabled? Also can we make the blue boxes' bottom be the same height as the blue boxes' top padding?

Otherwise the functionality of the text input 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.

5 participants