Standardize widget margins #5868
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alyblenkin
approved these changes
Dec 14, 2023
There was a problem hiding this comment.
@grzesiek2010 Sorry for the delay in coming back to this one! The question padding and margins, as well as the spacing between the questions look good!
seadowg
approved these changes
Dec 15, 2023
| ViewGroup.LayoutParams.WRAP_CONTENT, | ||
| ViewGroup.LayoutParams.MATCH_PARENT | ||
| ); | ||
| int marginVertical = (int) getContext().getResources().getDimension(org.odk.collect.androidshared.R.dimen.margin_small); |
There was a problem hiding this comment.
I did not realise that getDimension would give us back px rather than dp here. I guess that's because the dimension is defined as 8dp rather than just 8.
|
Tested with Success! Verified on Androids: 10 Verified cases:
|
|
Tested with Success! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #5731
Why is this the best possible solution? Were any other approaches considered?
The issue itself was only about adding more margins to dividers displayed between questions if we have multiple questions on one screen. I took the opportunity and reworked the way margins are added to questions. In the past, we would add margins to every single question separately which was error-prone and difficult to maintain. Now questions do not have margins but the main container that holds them does what is much easier.
@alyblenkin it would be good if you could take a look at the changes. Please pay attention not only to what the issue was about but also generally margins and padding in questions because maybe there is something else that we would need to change (in this pr or separately). When it comes to the space I've added to the dividers displayed between questions please keep in mind that it might be slightly different depending on what questions are above/below. For example, if the last element in a question is a button vs text field there might be a small difference visible because different components might have their own special margins/paddings and then along with the margin of the difider there might be more empty space. But as I said the difference should be not big.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This changes the way margins are added to questions so theoretically in every single question something might be broken. The good thing is that we don't need to test the functionality of questions only how they are displayed.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still pass