-
Notifications
You must be signed in to change notification settings - Fork 499
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 part of #1824: Add chapter dash line #3863
Conversation
@rt4914 @BenHenning Please review. |
Thanks @viktoriias. Sorry for not following up on Friday. I'll need one more day before I can take a look at this. In the mean time, could you resolve the conflicts & merge in the latest develop changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice. Please make following changes:
- Add
Fix part of #1824
in description - Add mobile/tablet scrrenshots in various configurations.
Otherwise LGTM
Done and done. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @viktoriias! This does look quite nice.
Had a couple of comments, plus a few additional questions:
- Can you please update the PR description to explain what's left in Lowfi: Tablet: StoryActivity Landscape + Portrait #1824 after this is merged?
- Follow-up from (1), do we still need to implement the empty circles & later dashed lines per the mocks for not-yet-completed chapters, or should that be completed in this PR? I'm asking because per your screenshots it seems that the line doesn't go to the bottom of the chapter list yet,
- Follow-up from (3), if this isn't complete yet, we should hide it behind a platform parameter flag until it's ready to be enabled (just to avoid a half-complete thing being merged)
- Was Lowfi: Tablet: StoryActivity Landscape + Portrait #1824 (comment) accounted for in this PR, or would that happen in a follow-up?
- Could you please update this PR with the latest develop & resolve the conflict?
app/src/main/java/org/oppia/android/app/customview/VerticalDashedLineView.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/databinding/ImageViewBindingAdapters.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/databinding/ImageViewBindingAdapters.java
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/databinding/ImageViewBindingAdaptersTest.kt
Show resolved
Hide resolved
…hedLineView.kt Co-authored-by: Ben Henning <henning.benmax@gmail.com>
Sorry, will need to review this tomorrow. |
Also @viktoriias there are failing CI checks--are they related to the latest changes? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @viktoriias. Just had a few follow-up comments, otherwise this is looking really good.
Re: the failure, it's probably #2844 and unrelated to this PR. Just to be sure, though, I re-ran the workflow to see if it will now pass.
app/src/sharedTest/java/org/oppia/android/app/databinding/BUILD.bazel
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/databinding/BUILD.bazel
Outdated
Show resolved
Hide resolved
Hmm it failed again, interesting. @viktoriias does it pass for you locally without issue? If possible, could you try running the same set of tests as the failing shard to see if that also passes for you? Specifically:
If this is CI-specific, we may need to introduce a retry mechanism also on the run step (which I've been considering to work around this specific flake). I can do that in a separate PR, but I first want to see if this is specific to CI. |
Checks are passing now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @viktoriias. This looks really good!
Merging since approvals seem complete & CI is passing. |
Explanation
Fix #1824.
This is the updated copy of #3066
What's new:
ImageViewBindingAdaptersTest
.Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: