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

Fixes #166: Story activity UI structure #195

Merged
merged 51 commits into from
Nov 7, 2019

Conversation

hareshkh
Copy link
Contributor

@hareshkh hareshkh commented Sep 29, 2019

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 assigned to an appropriate reviewer.

@hareshkh hareshkh changed the base branch from develop to introduce-data-binding-recycler-view-adapter September 29, 2019 18:42
@hareshkh
Copy link
Contributor Author

Changing base branch for now to show correct diff in the PR.

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 @hareshkh! This is a great start. Is this ready for review now?

I left a couple of preliminary comments without knowing if a deeper review is needed yet.

app/src/main/java/org/oppia/app/story/StoryActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/story/StoryActivity.kt Outdated Show resolved Hide resolved
@hareshkh
Copy link
Contributor Author

@BenHenning, the PR is not ready for a deeper review yet, I have to add the RecyclerView. I am reading about a few things. I will try to do it ASAP.

@BenHenning
Copy link
Member

Hi @hareshkh. Have you made any recent progress on this?

@hareshkh hareshkh changed the base branch from introduce-data-binding-recycler-view-adapter to develop October 8, 2019 19:01
@BenHenning
Copy link
Member

BenHenning commented Oct 18, 2019

@hareshkh is this ready for me to look at again?

If so, can you please merge the latest develop into this branch and resolve the conflicts, then assign the PR to me when it's ready for review?

@hareshkh hareshkh assigned BenHenning and unassigned hareshkh Oct 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.

Thanks @hareshkh! Overall, the PR LGTM except the static state bit being added into StoryActivity. Let's resolve that.

Can you also attach some screenshots of what the new activity looks like in the initial comment of the PR to provide a visual reference?

app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/story/StoryFragment.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/story_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/story_header_view.xml Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned hareshkh and unassigned BenHenning Nov 3, 2019
@hareshkh hareshkh assigned BenHenning and unassigned hareshkh Nov 6, 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.

Thanks @hareshkh! I think we can simplify this slightly after merging in develop since there were some downstream changes to the home activity.

app/src/main/java/org/oppia/app/story/StoryActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/story/StoryFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/story/StoryViewModel.kt Outdated Show resolved Hide resolved
Conflicts:
	app/src/main/AndroidManifest.xml
	app/src/main/java/org/oppia/app/databinding/DrawableBindingAdapters.kt
	app/src/main/java/org/oppia/app/home/HomeFragmentPresenter.kt
	app/src/main/res/drawable/rounded_rect_background.xml
	app/src/main/res/layout/home_fragment.xml
	app/src/main/res/values/strings.xml
…android into story-activity-ui-structure

Conflicts:
	app/src/main/java/org/oppia/app/home/HomeFragmentPresenter.kt
	app/src/main/res/layout/home_fragment.xml
isolating story fragment tests), and miscellaneous tweaks.
@BenHenning
Copy link
Member

All remaining changes seem addressed. @rt4914 can you verify just the new commits briefly to make sure another pair of eyes verifies them?

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. Test cases are working.

Some nit changes.

app/build.gradle Outdated Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
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.

One test related comment

app/build.gradle Outdated Show resolved Hide resolved
@BenHenning
Copy link
Member

Thanks @rt4914 for the comments, and @hareshkh for following up. The fixes LGTM, going ahead and merging this.

@BenHenning BenHenning merged commit 2944a67 into develop Nov 7, 2019
@BenHenning BenHenning deleted the story-activity-ui-structure branch November 7, 2019 05:27
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