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

Make Spinner widget open from start-of-list when nothing is selected #1955

Merged
merged 2 commits into from Mar 2, 2018

Conversation

iammvaibhav
Copy link
Contributor

Closes #1935

What has been done to verify that this works as intended?

Used the form for testing provided In the Issue

Why is this the best possible solution? Were any other approaches considered?

AppCompatSpinner by default doesn't provide such functionality, so in order to achieve this, extending AppCompatSpinner was must.

Are there any risks to merging this code? If so, what are they?

I don't think so.

Do we need any specific form for testing your changes? If so, please attach one.

selectOneMinimal.xml.txt


/**
* Created by vaibhav on 1/3/18.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please replace this comment with a description of the class. The very top should have a copyright notice with your name (you are the copyright holder) -- https://github.com/opendatakit/collect/blob/master/LICENSE.md#appendix-how-to-apply-the-apache-license-to-your-work

Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Thanks, @iammvaibhav! I've made a few comments inline.

* Created by vaibhav on 1/3/18.
*/

public class CustomSpinner extends AppCompatSpinner {
Copy link
Member

Choose a reason for hiding this comment

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

What would a more descriptive name be? How about ScrolledToTopSpinner or something like that?


public class CustomSpinner extends AppCompatSpinner {

private boolean toggleFlag = true;
Copy link
Member

Choose a reason for hiding this comment

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

This should also have a more descriptive name. I would call it spinnerClicked and have it start as false. It's confusing that the condition in getSelectedItemPosition is negated.

@iammvaibhav
Copy link
Contributor Author

Thank You @lognaturel for suggestions. I have made the required changes.

@iammvaibhav
Copy link
Contributor Author

iammvaibhav commented Mar 1, 2018

I have no idea why build_debug is failing on CircleCI. The logs aren't helpful either.
I ran the same task ./gradlew pmd checkstyle lint findbugs on my computer and the build was successful.

Can you please point out any errors?

@mmarciniak90
Copy link
Contributor

Tested with success!

Verified on Android: 4.1, 4.2, 4.4, 5.1, 6.0, 7.0, 8.0.

Verified cases:

  • new form
  • saved form
  • both device orientations

New, related issue was created - #1960

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

@lognaturel lognaturel merged commit d1b5685 into getodk:master Mar 2, 2018
@lognaturel
Copy link
Member

Thanks, @iammvaibhav! The failure was due to the continuous integration server running out of memory. I simply re-ran it and it passed.

shobhitagarwal1612 pushed a commit to shobhitagarwal1612/collect that referenced this pull request May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants