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 part of #2003: Modified the use of space_20 #2010

Merged
merged 4 commits into from
Oct 17, 2020

Conversation

Arjupta
Copy link
Contributor

@Arjupta Arjupta commented Oct 14, 2020

Explanation

Fixes part of #2003
This commit removes the use of dimension space_20 with the dimensions of use-case specific names.

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.

@Arjupta Arjupta requested a review from rt4914 October 14, 2020 20:18
@rt4914
Copy link
Contributor

rt4914 commented Oct 14, 2020

@Arjupta considering that you are now a collaborator. Please make sure that you Keep the Assignnees section up-to-date as per these guidelines.
https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

1 similar comment
@rt4914
Copy link
Contributor

rt4914 commented Oct 14, 2020

@Arjupta considering that you are now a collaborator. Please make sure that you Keep the Assignnees section up-to-date as per these guidelines.
https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

@rt4914
Copy link
Contributor

rt4914 commented Oct 14, 2020

Also, @Arjupta You can work on other issues meanwhile we start discussion about the the naming on this PR with other team mates.

@BenHenning Can you have a look at the naming convention for dimensions and leave your thoughts?

@Arjupta
Copy link
Contributor Author

Arjupta commented Oct 15, 2020

@Arjupta considering that you are now a collaborator. Please make sure that you Keep the Assignnees section up-to-date as per these guidelines.
https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section

Okay, I will follow them from now on. Mistakenly closed this PR while using the Github Mobile App

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 @Arjupta & @rt4914. I find these much clearer. Left some comments on the specific naming.

app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/walkthrough_topic_summary_view.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/profile_chooser_profile_view.xml Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned rt4914 and Arjupta and unassigned BenHenning Oct 15, 2020
@aggarwalpulkit596
Copy link
Contributor

@Arjupta could you assign me as well for all of your future PR's.
Thanks

@Arjupta
Copy link
Contributor Author

Arjupta commented Oct 16, 2020

@Arjupta could you assign me as well for all of your future PR's.
Thanks

Okay, I will take care of that.

@Arjupta Arjupta removed the request for review from rt4914 October 16, 2020 21:38
@Arjupta
Copy link
Contributor Author

Arjupta commented Oct 16, 2020

@BenHenning PTAL

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.

The names LGTM. Thanks!

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.

@Arjupta One nit and rest all is good.

app/src/main/res/values-land/dimens.xml Outdated Show resolved Hide resolved
@Arjupta
Copy link
Contributor Author

Arjupta commented Oct 17, 2020

@rt4914 I think this is ready to merge, shall I proceed with PR related to other dimensions now?

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.

You can start creating other parts but make sure they are still smaller PRs as it will create less merge conflicts and fast approvals.

Roughly I think you can change nearly 10-15 files for such issues in single PR.

@rt4914 rt4914 merged commit 37f64ff into oppia:develop Oct 17, 2020
@Arjupta
Copy link
Contributor Author

Arjupta commented Oct 17, 2020

LGTM, thanks.

You can start creating other parts but make sure they are still smaller PRs as it will create less merge conflicts and fast approvals.

Roughly I think you can change nearly 10-15 files for such issues in single PR.

Sure will take care of that. Can I know whether I should ask for a review on this issue from you or Ben?

Thanks

@rt4914
Copy link
Contributor

rt4914 commented Oct 17, 2020

LGTM, thanks.
You can start creating other parts but make sure they are still smaller PRs as it will create less merge conflicts and fast approvals.
Roughly I think you can change nearly 10-15 files for such issues in single PR.

Sure will take care of that. Can I know whether I should ask for a review on this issue from you or Ben?

Thanks

You can ask me and if I will have any doubt I will add @BenHenning too.

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.

4 participants