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.

Sign up for GitHub

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

Jump to bottom

Fixes part of #1159 & #1161: Topic-Lessons high-fi animation #1465

Merged
merged 12 commits into from
Jul 28, 2020
Merged

Fixes part of #1159 & #1161: Topic-Lessons high-fi animation #1465

merged 12 commits into from
Jul 28, 2020

Conversation

MohamedMedhat1998
Copy link
Contributor

@MohamedMedhat1998 MohamedMedhat1998 commented Jul 13, 2020 •
edited
Loading

Explanation

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.

rt4914
rt4914 suggested changes Jul 14, 2020
View reviewed changes
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.

The approach looks good.
I have suggested some nit changes.

I had few concerns:

  1. When I click on the icon to rotate, the entire click blink once.
  2. I tried to run this in tablet and it worked but it was not working in mobile. Can you please check that.
  3. Once this is approved/merged, you can update Hints & Solution and get that reviewed too.

app/src/main/java/org/oppia/app/databinding/ImageViewBindingAdapters.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Jul 14, 2020
@MohamedMedhat1998 MohamedMedhat1998 mentioned this pull request Jul 15, 2020
Closed
8 tasks
@MohamedMedhat1998
Copy link
Contributor Author

@rt4914 please check the blinking issue!

@rt4914
Copy link
Contributor

rt4914 commented Jul 15, 2020

@rt4914 please check the blinking issue!

@MohamedMedhat1998 To me it seems that the blinking has stopped, the rotation animation is working but the expand/collapse animation is not working. Can you please confirm if you observe same behaviour? Also, if this issue is seen, then I would suggest to leave this PR for now and wait for #1359 to get merged which might help you in expand/collapse animation.

@rt4914 rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Jul 15, 2020
@MohamedMedhat1998
Copy link
Contributor Author

MohamedMedhat1998 commented Jul 15, 2020 •
edited
Loading

@rt4914 please check the blinking issue!

@MohamedMedhat1998 To me it seems that the blinking has stopped, the rotation animation is working but the expand/collapse animation is not working. Can you please confirm if you observe same behaviour? Also, if this issue is seen, then I would suggest to leave this PR for now and wait for #1359 to get merged which might help you in expand/collapse animation.

Yes, that is what I'm having too and it is an expected behavior in this approach.
We aren't expanding nor collapsing, we are moving the other items around the target item to be expanded/collapsed, and hide/show the details section of the target item.
For now, I'll leave this PR, please tell me when #1359 gets merged

@rt4914 rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Jul 22, 2020
@MohamedMedhat1998
Merge remote-tracking branch 'origin/develop' into t-lessons-animation
e16d838 
# Conflicts (Resolved):
#	app/src/main/res/layout-sw600dp-land/topic_lessons_story_summary.xml
#	app/src/main/res/layout-sw600dp-port/topic_lessons_story_summary.xml
@MohamedMedhat1998
-generalized the arrow animation
c657f77
@MohamedMedhat1998
Copy link
Contributor Author

@rt4914 That is the best implementation I could make. I'll create a new branch from this one and try merging with #1359 as suggested

rt4914
rt4914 suggested changes Jul 28, 2020
View reviewed changes
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.

This looks really good to me. Please address one nit suggestion.

app/src/main/java/org/oppia/app/databinding/ViewBindingAdapter.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned MohamedMedhat1998 and unassigned rt4914 Jul 28, 2020
rt4914
rt4914 approved these changes Jul 28, 2020
View reviewed changes
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 merged commit e7b801d into develop Jul 28, 2020
@rt4914 rt4914 deleted the t-lessons-animation branch July 28, 2020 17:35
This was referenced Jul 29, 2020
Closed
Closed
@rt4914 rt4914 mentioned this pull request Aug 6, 2020
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MohamedMedhat1998 @rt4914 @BenHenning