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 #1646:Changed all padding_# and margin_# to space_#dp in dimens value. #1936

Merged
merged 4 commits into from
Oct 7, 2020

Conversation

ranjsa
Copy link
Contributor

@ranjsa ranjsa commented Oct 6, 2020

Explanation

Fixes #1646

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.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @ranjsa
Let's wait for Rajat comment on dpdp thing and then you can update it accordingly.

Added a few points and also added 1 point to clarify between padding_72 vs padding_72dp.

app/src/main/res/layout/walkthrough_final_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/walkthrough_final_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Show resolved Hide resolved
@anandwana001 anandwana001 removed their assignment Oct 6, 2020
@ranjsa
Copy link
Contributor Author

ranjsa commented Oct 6, 2020

Nice work @ranjsa
Let's wait for Rajat comment on dpdp thing and then you can update it accordingly.

Added a few points and also added 1 point to clarify between padding_72 vs padding_72dp.

Thank You @anandwana001 for reviewing my PR. I have fixed all space_#dpdp to space_#dp.

app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@anandwana001 anandwana001 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 @ranjsa
@rt4914 can take a quick pass on this and then we are good to merge this. By the time, feel free to check other issues to work on.

@ranjsa
Copy link
Contributor Author

ranjsa commented Oct 6, 2020

LGTM, Thanks @ranjsa
@rt4914 can take a quick pass on this and then we are good to merge this. By the time, feel free to check other issues to work on.

Okay sure, Thanks @anandwana001

@BenHenning
Copy link
Member

Ideally, we should be semantically naming all dimensions. @rt4914 would you prefer that we check this PR in as-is and fix that in a later change?

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.

Thanks a lot @ranjsa . This is really great.

Also, I understand that its not easy to make a change and resolve merge conflicts again and again and therefore I am approving this so that if you want to merge this PR at this stage you can do that.

If you choose to make the suggested change, then just assign it back to me and I review back and merge.

If you choose to merge it right now, then I suggest you file an issue here . You can also add this under SLoP tags too.

Sounds good?

@rt4914
Copy link
Contributor

rt4914 commented Oct 7, 2020

Ideally, we should be semantically naming all dimensions. @rt4914 would you prefer that we check this PR in as-is and fix that in a later change?

@BenHenning I am not 100% sure what you mean here?
(a.) Is it about reordering or the space_ values in dimens.xml file
or (b.) The naming should be changed to provide more context to paddings/margins?
or something else.

@rt4914 rt4914 assigned ranjsa and unassigned rt4914 Oct 7, 2020
@ranjsa
Copy link
Contributor Author

ranjsa commented Oct 7, 2020

Hello, @rt4914 thanks you for reviewing this PR.
Also, did you mean creating a new issue for this #1936 (comment)
#1936 (comment) if so I would like to work on this issue on a different PR if I am right then please confirm once so I can proceed.

@rt4914
Copy link
Contributor

rt4914 commented Oct 7, 2020

Hello, @rt4914 thanks you for reviewing this PR.
Also, did you mean creating a new issue for this #1936 (comment)
#1936 (comment) if so I would like to work on this issue on a different PR if I am right then please confirm once so I can proceed.

Yeah, you should file a new issue.
I will merge this PR now and you can file a new issue and start working on it.

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.

Make dimens value follow one naming convention
4 participants