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 #2658, #299: Replace builder() with Factory in SingleTypeBuilder and MultiTypeBuilder #4412

Conversation

KevinGitonga
Copy link
Contributor

@KevinGitonga KevinGitonga commented Jul 1, 2022

Fixes #2658
Fixes #299

This PR includes a set of changes to the BindableAdapter including creating a Factory Instance of the SingleTypeAdapter and MultiTypeAdapter instead of the currently used Builder Pattern to enable it to be Injected into Fragments and ListView instances used throughout the App.

See also KevinGitonga#1 for an explanation on some sub-parts of the PR.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

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 @KevinGitonga. At a high-level, I think we need to be injecting the factory since that's the intent of the issue being fixed.

For the PR itself:

  • Please make sure that the title is complete (it shouldn't be abbreviated, and probably needs more clarity on what the PR is actually doing), and follows the format: Fix #<issuenumber>: Title.
  • Please make sure the PR description includes Fixes #<issuenumber> as part of the explanation. I also suggest rewording the explanation to be clearer on what's actually being solved, and why the solution makes sense.
  • If the UI section doesn't need to be filled out, please include a brief justification for why it doesn't.
  • It looks like a bunch of CI checks are failing. Once you address my comments, I suggest going through and making sure that those checks are passing before sending the PR back for review.

@BenHenning BenHenning assigned KevinGitonga and unassigned BenHenning Jul 1, 2022
@BenHenning
Copy link
Member

@KevinGitonga given that this PR is still in draft, did you mean for me to perform another review pass? It's not clear what you're looking for here since you didn't leave an explicit ask message on the PR (or maybe I missed it?).

If the PR is ready for full review, you should click the 'Ready for review' button. If it's still in draft, please follow up with what you'd like me to specifically look at.

@BenHenning BenHenning removed their assignment Jul 14, 2022
@KevinGitonga KevinGitonga marked this pull request as ready for review July 14, 2022 17:35
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 @KevinGitonga! I think this PR is definitely on the right track, but I noticed a bunch of places that will need some changes. PTAL.

@KevinGitonga KevinGitonga removed their assignment Jul 15, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 23, 2022
@BenHenning
Copy link
Member

BenHenning commented Sep 24, 2022

@KevinGitonga PTAL at KevinGitonga#1 and added comments or merge, then assign this back to me.

Also, if you decide to merge it please also verify that it addresses your issue as expected.

Update PR oppia#4412 with various fixes [Blocked: #2]
…_chapter_view.xml which is not available on my branch even after several sync's causing build errors.
@KevinGitonga
Copy link
Contributor Author

@BenHenning have reviewed have however noticed a few failing tests that are unrelated to this pr, that were picked up during merging of changes probably. PTAL and advice. Should i revert them or will be auto resolved when merging to develop.

@BenHenning
Copy link
Member

It looks like the failure has come from a bad conflict resolution in TopicLessonsFragmentPresenter after updating the branch to the latest develop changes. Some deltas were lost and need to be restored in order for that file to be correct (at the moment, it's actually regressing so the test failures are correct).

@KevinGitonga
Copy link
Contributor Author

@BenHenning reverted the deleted deltas which fixes the previously failing tests PTAL.

@BenHenning BenHenning changed the title Fix #2658: Replace builder() with Factory in SingleTypeBuilder and MultiTypeBuilder Fix #2658, #299: Replace builder() with Factory in SingleTypeBuilder and MultiTypeBuilder Oct 1, 2022
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 @KevinGitonga. Only had one nit that I think we need to address before the PR can be merged.

@BenHenning BenHenning assigned KevinGitonga and unassigned BenHenning Oct 1, 2022
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 @KevinGitonga. This LGTM!

@BenHenning BenHenning merged commit 8594c9d into oppia:develop Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants