-
Notifications
You must be signed in to change notification settings - Fork 73
Improvements in bottom bar layout #235
base: master
Are you sure you want to change the base?
Conversation
@shobhitagarwal1612 @lakshyagupta21 I made the suggestions given by you can you have a look now |
@lakshyagupta21 as you suggested that background color white is looking odd I tried some of the other colors similar to the primary color of the app but it does not looks good |
The disabled buttons look just like enabled. Please fix |
Initially, I made in the color of gray but @lakshyagupta21 suggested me to make it into app primary color so I made like that, can you suggest me color to make it |
The text color should be grayed out similar to the original button's implementation. |
@shobhitagarwal1612 @lakshyagupta21 I made the changes suggested by you and I updated how it looks like in PR template can you please review 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.
The buttons look great now and work as expected. Nice work @Chromicle 👍
Just a small nitpick and this is good to be merged
|
||
<item> | ||
<shape> | ||
<solid android:color=" #D3D3D3" /> |
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.
Please fix this spacing and add the color to the colors.xml
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
android:layout_alignParentBottom="true" | ||
android:orientation="horizontal" | ||
android:weightSum="2"> | ||
android:background="#ffffff" |
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.
Please avoid using raw values wherever possible.
This can be replaced with @android:color/white
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 instead of @android: color/white
I used @android: color/colorTabActive
as this is initialized before only
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.
@Chromicle Sometimes variable name or resources name may confuse someone(as in this case colorTabActive is reserved for tabs related activity but if someone in future changes this value then it will reflect here as well) so its better to use @android:color/white
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.
@Chromicle Sometimes variable name or resources name may confuse someone(as in this case colorTabActive is reserved for tabs related activity but if someone in future changes this value then it will reflect here as well) so it's better to use
@android:color/white
I made the changes can you have a look now
@@ -5,4 +5,5 @@ | |||
<color name="colorAccent">#00796B</color> | |||
<color name="colorTabInactive">#CFD8DC</color> | |||
<color name="colorTabActive">#FFFFFF</color> | |||
<color name="lightGray">#D3D3D3</color> |
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.
Give some meaningful name(for e.g. if you're using it for button border then name it buttonBorder
)
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.
yeah, I named it to colorButtonDisabled
@@ -193,8 +193,12 @@ private void toggleButtonLabel() { | |||
} | |||
if (selectedForms.isEmpty()) { | |||
sendButton.setText(getString(R.string.send_forms)); | |||
sendButton.setBackground(getResources().getDrawable(R.drawable.selector_bottom_recive)); |
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.
sendButton.setText(getString(R.string.send_forms));
sendButton.setBackground(getResources().getDrawable(R.drawable.selector_bottom_recive));
sendButton.setTextColor(getResources().getColor(R.color.colorPrimary));
It will be good for you to extract those similar code to a function like updateSendButton(String newText, @DrawableRes int newDrawableRes, @ColorRes int newTextColor)
.
@@ -21,37 +21,66 @@ | |||
android:layout_centerHorizontal="true" | |||
android:layout_centerVertical="true" | |||
android:gravity="center" | |||
android:textSize="22sp" | |||
android:padding="10dp"/> | |||
android:padding="10dp" |
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.
avoid hard coding is better, you can replace the padding by @dimen/default_padding_2.5x
.
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.
Until now we have not used @dimen
so made like hardcoded or else I will make into that way only
android:layout_weight="1" | ||
android:gravity="center" | ||
android:paddingStart="10dp" | ||
android:paddingLeft="10dp" |
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.
using padding End
will make your life easier when facing RTL languages.
@@ -11,43 +11,65 @@ | |||
android:layout_centerHorizontal="true" | |||
android:layout_centerVertical="true" | |||
android:gravity="center" | |||
android:textSize="22sp" | |||
android:padding="10dp"/> | |||
android:padding="10dp" |
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.
The same as above.
android:gravity="center" | ||
android:padding="10dp"> | ||
|
||
<Button |
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.
toggle_button
has so many attrs the same as send_button
, consider extract them into style.xml
.
@@ -5,4 +5,6 @@ | |||
<color name="colorAccent">#00796B</color> | |||
<color name="colorTabInactive">#CFD8DC</color> | |||
<color name="colorTabActive">#FFFFFF</color> | |||
<color name="colorButtonDisabled">#D3D3D3</color> | |||
<color name="white">#FFFFFF</color> |
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.
We discussed this @Chromicle in some other PR that avoid naming color resources with the actual color names, give some meaningful names to them, In this case we can remove the white color resource as you can directly use it like android:color/white
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.
yeah! I updated
@Chromicle Can you resolve the conflicts? |
@lakshyagupta21 @shobhitagarwal1612 I have done the changes and I added the styles instead of adding same to code to everywhere and I did testing twice and it is working perfectly as accepted |
@lakshyagupta21 @huangyz0918 can you please review this PR as there are a lot of file changes every time new PR merges I have to solve the conflict |
Closes #174
What has been done to verify that this 4works as intended?
Tested in Android Studio and my phone(Samsung j8), it looks good as I expected
GIF
Why is this the best possible solution
Made 2 drawable resource files so 2 buttons look different and applied the background to the buttons also. and added the same layout to fill forms and blank forms and main activity also
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?
As by updating the UI so it holds good for user experience and so the app looks like beautiful
Before submitting this PR, please make sure you have:
./gradlew pmd checkstyle lint findbugs
and confirmed all checks still pass OR confirm CircleCI build passes