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

UI Updates to Tap Detail View #124

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

johnnyruz
Copy link

Here are some updates I made a while ago to add some additional information (ABV/IBU) to the main tap display, as well as clean up some text spacing issues I was experiencing on my Acer A210.

There are a couple things going on with that tablet, mainly that the Android system text size defaults to "Large" which throws things off. Setting to "Normal" looks better but I still adjusted the size so that values are still readable but don't overflow their containers.

Would love to have some additional testing and review on newer devices, almost certainly with higher resolutions.

Copy link
Collaborator

@patfreeman patfreeman left a comment

Choose a reason for hiding this comment

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

I like getting the ABV and IBU on the screen. I'm open to suggestions on the language of those stats.
Some minor sizing helped make it look better.
The other layout changes need some work to look good on more devices.

Comment on lines 44 to 64
<CheckBoxPreference
android:title="Show ABV when zero"
android:key="config:ABV_DISPLAY_WHEN_ZERO"
android:summaryOn="ABV will be displayed even when zero"
android:summaryOff="ABV will not be displayed when zero"
android:defaultValue="false">
</CheckBoxPreference>
<CheckBoxPreference
android:title="Show IBU when zero"
android:key="config:IBU_DISPLAY_WHEN_ZERO"
android:summaryOn="IBU will be displayed even when zero"
android:summaryOff="IBU will not be displayed when zero"
android:defaultValue="false">
</CheckBoxPreference>
<CheckBoxPreference
android:title="Show Tap Notes"
android:key="config:DISPLAY_TAP_NOTES"
android:summaryOn="Tap Notes will be displayed if available"
android:summaryOff="Tap Notes will not be displayed"
android:defaultValue="true">
</CheckBoxPreference>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be in the User Interface section of settings_kegerator.xml

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, yeah I could see it making more sense on that page since that grouping (currently only one item) controls the tap detail display. Makes sense under the title of "Look and Feel", but maybe that should just be "Units" and the UI settings are under Kegerator > User Interface

<item name="android:layout_height">wrap_content</item>
<item name="android:ellipsize">end</item>
<item name="android:singleLine">true</item>
<item name="android:textSize">22sp</item>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was a touch too big for the default font sizes on my nexus 7. ABV and IBU lines bled together. 20sp looked better to me.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, should also look fine on my 10" tablet and will give just a bit extra room for Beer Name/Tap Name. Definitely tricky trying to get a solution that works on all screen sizes. I need to refresh my memory on the units 'sp'/'dip'. Maybe there's a better way ensure consistent UI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sp is supposed to be the better way at least according to what android studio just told me. :)

// Find ABV and IBU values
final String abv = String.valueOf(keg.getBeverage().getAbvPercent());
if(mCore.getConfiguration().getAbvVisibleWhenZero()) {
abvText.setText("ABV: " + abv + "%");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd personally prefer it to read more naturally, like 6.7% ABV

Copy link
Author

@johnnyruz johnnyruz May 19, 2020

Choose a reason for hiding this comment

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

I could agree with that, thoughts on also flipping the IBU display? I think having both labels as a prefix or suffix would look best vs. having one as 6.7% ABV and the other IBU: 50. EDIT: Missed the comment below!

if(mCore.getConfiguration().getAbvVisibleWhenZero()) {
abvText.setText("ABV: " + abv + "%");
} else{
abvText.setText(keg.getBeverage().getAbvPercent() == 0 ? "" : "ABV: " + abv + "%");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably just set the visibility to GONE. abvText.setVisibility(View.GONE);

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, fixing

if(mCore.getConfiguration().getIbuVisibleWhenZero()){
ibuText.setText("IBU: " + ibu);
} else{
ibuText.setText(keg.getBeverage().getIbu() == 0 ? "" : "IBU: " + ibu);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably just set the visibility to GONE. ibuText.setVisibility(View.GONE);

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, fixing


final String ibu = String.valueOf(Math.round(keg.getBeverage().getIbu()));
if(mCore.getConfiguration().getIbuVisibleWhenZero()){
ibuText.setText("IBU: " + ibu);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd personally prefer it to read more naturally, like 17 IBUs

Copy link
Author

Choose a reason for hiding this comment

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

On it!

android:layout_height="wrap_content"
android:layout_below="@+id/tapStatsBadges"
android:layout_centerHorizontal="true"
android:layout_margin="16dp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like a huge margin.

android:layout_centerHorizontal="true"
android:layout_margin="16dp"
android:layout_marginTop="2dip"
android:padding="16dp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

and padding.

Copy link
Author

Choose a reason for hiding this comment

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

Looked ok on my tablet...but looks better with it eliminated/reduced! Fixed!

@@ -38,7 +38,7 @@

<!-- Right box: Camera Preview & Controls -->

<LinearLayout
<RelativeLayout
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change hide the Done Pouring button.

Copy link
Author

Choose a reason for hiding this comment

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

Aligned camera view to bottom but reverted changes to Done Button/Image/Shout text to maintain visibility on smaller tablet screens.

@mik3y
Copy link
Member

mik3y commented May 21, 2020

just let me if/when this is ready to go, gents!

@mik3y
Copy link
Member

mik3y commented Jun 28, 2020

hey gents! just checking in here, does this still need work? - take your time if so, just making sure you're not waiting on me.

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.

None yet

3 participants