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.

Sign up for GitHub

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

Jump to bottom

feat : Migrate hardcoded dimensions to value file #409

Closed
wants to merge 2 commits into from
Closed

feat : Migrate hardcoded dimensions to value file #409

wants to merge 2 commits into from

Conversation

Ana2k
Copy link
Contributor

@Ana2k Ana2k commented Mar 30, 2021 •
edited
Loading

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straighforward changes).

Fixes #[407]

Description
Migrated hardcoded dimension values from XML files to value files.

Alternative(s) considered
N/A

Type
Choose one: (Code health)

Screenshots (if applicable)

Checklist

  • I have read and acknowledge the Code of conduct
  • I have read How to Contribute
  • I have read the Developer's guide
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
  • I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

@Ana2k Ana2k changed the title [draft] : Migrate hardcoded dimensions to value file feat : Migrate hardcoded dimensions to value file Mar 30, 2021
@Ana2k Ana2k marked this pull request as ready for review March 30, 2021 18:48
@Ana2k
Copy link
Contributor Author

Ana2k commented Mar 30, 2021

@jingtang10 I have migrated the hardcoded dimensions to value files. Please review this pr. Thanks!

@jingtang10 jingtang10 linked an issue Mar 30, 2021 that may be closed by this pull request
Migrate hardcoded dimensions to dimens file #407
Closed
jingtang10
jingtang10 requested changes Apr 3, 2021
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks for your contribution.

cqlreferece and referece UI is going to change a lot and I really don't think extracting dimens values is a priority. please revert those changes.

datacapture/src/main/res/layout/questionnaire_item_date_time_picker_view.xml
@@ -25,7 +25,7 @@
>

<LinearLayout
android:layout_width="0dp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

0dp is such a special value (work with layout weight) I don't see the value of creating a dimen value.

datacapture/src/main/res/layout/questionnaire_item_date_time_picker_view.xml
@@ -78,7 +78,7 @@
/>

<LinearLayout
android:layout_width="0dp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

datacapture/src/main/res/values/dimens.xml
@@ -15,6 +15,7 @@
limitations under the License.
-->
<resources>
<dimen name="width_null">0dp</dimen>
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this.

datacapturegallery/src/main/res/values/dimens.xml
@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8" ?>
<resources>
<dimen name="padding_large">16dp</dimen>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<dimen name="padding_large">16dp</dimen>
<dimen name="padding_list_item">16dp</dimen>

@jingtang10
Copy link
Collaborator

please remove "feat" in the PR title since this is not a feature.

@Ana2k
Copy link
Contributor Author

Ana2k commented Apr 4, 2021

thanks for your contribution.

cqlreferece and referece UI is going to change a lot and I really don't think extracting dimens values is a priority. please revert those changes.

Thanks for the heads up. I will surely make the suggested changes.
I had a question, What are the high priority issues for the upcoming weeks?
Does testing of datacapture or further relevant changes in the datacapture module fall under this?

@jingtang10
Copy link
Collaborator

@Ana2k apologies but I think it's actually best to close this PR and the issue itself. If you are keen to contribute please pick up some other tasks.

@jingtang10 jingtang10 closed this Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingtang10 jingtang10 jingtang10 requested changes

@deepankarb deepankarb Awaiting requested review from deepankarb

@joiskash joiskash Awaiting requested review from joiskash

@kevinmost kevinmost Awaiting requested review from kevinmost

@pankajkatoch pankajkatoch Awaiting requested review from pankajkatoch

@yigit yigit Awaiting requested review from yigit

Assignees
No one assigned
Labels
None yet
Projects
Android FHIR SDK
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate hardcoded dimensions to dimens file
2 participants
@Ana2k @jingtang10