-
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 # 3581 Merge topic_lessons_story_summary.xml #4234
Conversation
PTAL @bkaur-bkj |
PTAL @rt4914 |
<dimen name="option_audio_language_padding_start">16dp</dimen> | ||
<dimen name="option_audio_language_padding_end">16dp</dimen> | ||
|
||
<!--TopicLessonsSummary--> |
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.
@shankarpriyank write this as <!-- TopicLessonsSummary -->
do this for all dimen files
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.
Done
<!-- OptionAudioLanguage --> | ||
<dimen name="option_audio_language_padding_start">16dp</dimen> | ||
<dimen name="option_audio_language_padding_end">16dp</dimen> | ||
|
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.
@shankarpriyank also this part is maybe copied by you by mistake as you are not using this OptionAudiolanguage dimens anywhere in your files
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.
Sorry, I guess this happened while resolving the merge conflicts, I will make fix it
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.
Actually this looks a bit weird , my pr which was merged #4220 , I added the dimens for the OptionAudioLanguage in both the dimen files for the tablet , but right now in the develop branch I can't see them , they were deleted in the last commit through this pr #4226 , I skimmed through it once I feel maybe these values may have been deleted by mistake in that commit
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.
@shankarpriyank I actually don't think that there would have been such a big mistake in any other PR else it would not have been merged. I think you should just update your code to develop and keep only the changes required for this PR. PTAL @rt4914
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.
okay, @bkaur-bkj its updated now
PTAL @bkaur-bkj |
Unassigning @shankarpriyank since a re-review was requested. @shankarpriyank, please make sure you have addressed all review comments. Thanks! |
Hi @bhaktideshmukh I have resolved the issues as requested by Rajat, but I am having a hard time resolving the merge conflict as it says |
Please refer to my reply in another of your PR which had similar conflicts. |
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.
LGTM. Just need to resolve merge conflicts and it should be ready to merge.
Assigning @bkaur-bkj for code owner reviews. Thanks! |
@shankarpriyank |
PTAL @bkaur-bkj |
@rt4914 Done |
Unassigning @shankarpriyank since a re-review was requested. @shankarpriyank, please make sure you have addressed all review comments. Thanks! |
PTAL @bhaktideshmukh |
LGTM |
PTAL @rt4914 |
Hi @shankarpriyank, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
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.
LGTM. Just merge with latest develop and it should be ready to merge.
@rt4914 Done |
Hi @shankarpriyank, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Hi @shankarpriyank, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Explanation
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: