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 for issue #1436 list icon allows user only to show the list, but not to make it disappear #1443

Merged
merged 2 commits into from
Apr 17, 2018

Conversation

brok85
Copy link
Contributor

@brok85 brok85 commented Apr 14, 2018

…the current state of the list. It is false by default when you open the nearby activity.

Description

Fixes #1436

{Describe the changes made and why they were made.}
Created a boolean variable that defaults to false (because the list is collapsed by default) when the nearby activity is opened. Then, it changes value according to the current state of the list.

Tests performed

Tested on LG V10, API 24, master 2.7.x
Tested on {API level & name of device/emulator}, with {build variant, e.g. ProdDebug}.

{Please test your PR at least once before submitting.}
tested several times and seems to work properly.

Screenshots showing what changed

…the current state of the list. It is false by default when you open the nearby activity.
@codecov-io
Copy link

codecov-io commented Apr 14, 2018

Codecov Report

Merging #1443 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1443      +/-   ##
=========================================
- Coverage     3.3%    3.3%   -0.01%     
=========================================
  Files         128     128              
  Lines        6839    6842       +3     
  Branches      669     671       +2     
=========================================
  Hits          226     226              
- Misses       6598    6601       +3     
  Partials       15      15
Impacted Files Coverage Δ
...ava/fr/free/nrw/commons/nearby/NearbyActivity.java 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b012ce4...508fd24. Read the comment docs.

@neslihanturan
Copy link
Collaborator

Thanks for this PR @brok85 , just after we decide this button is a good idea, I will test and merge this.

// Handle item selection
switch (item.getItemId()) {
case R.id.action_display_list:
bottomSheetBehaviorForDetails.setState(BottomSheetBehavior.STATE_HIDDEN);
bottomSheetBehavior.setState(BottomSheetBehavior.STATE_EXPANDED);
if(!opened){
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would remove "opened" variable at all, since someone else may forgot to set it, and everything can be a mess. Instead you can use bottomSheetBehavior.getState(), to check if it is opened or not.

@neslihanturan
Copy link
Collaborator

Thanks for your first time contribution @brok85 , I will review this again just after you fixed reviewed point.

@brok85
Copy link
Contributor Author

brok85 commented Apr 16, 2018

@neslihanturan thanks for the suggestion. I pushed an updated version using getState.

the bottomSheetBahavior state technically can be different from hidden, collapsed, or expanded. It can also be "dragging" or "settling" but I don't thing those two states are ever assigned in this case. That is why they are not included in the new if statement.

Let me know if the new version is ok or if I should make other changes.

@neslihanturan
Copy link
Collaborator

Thanks @brok85 !

@neslihanturan neslihanturan merged commit 01a11a9 into commons-app:master Apr 17, 2018
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.

3 participants