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

Button styling improvements #5773

Merged
merged 6 commits into from
Nov 8, 2023
Merged

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Oct 10, 2023

Closes #5735

Why is this the best possible solution? Were any other approaches considered?

Nothing to discuss here.

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?

Things that I have changed:

  • removed the bold font from buttons
  • updated the color of borders in outline buttons

Please review all buttons to make sure they look as expected.

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:

  • added or modified tests for any new or changed behavior
  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • added a comment above any new strings describing it for translators
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010
Copy link
Member Author

@alyblenkin please take a look at the changes and let me know what you think about the new font size. I used the same size as we already had in the main menu buttons as @lognaturel proposed.
In some cases it might be a problem for example:
Screenshot_1696924223
but it didn't look perfect with the smaller size too so we might need to think about doing something with those buttons or at last adding shorter labels maybe?

@alyblenkin
Copy link
Collaborator

@grzesiek2010

First of all, removing the bold font makes it so much easier to read!

I think the button font size for the menu works really well. However, in the form-filling experience, the increased font size of the buttons in comparison to the question font size looks too drastically different. And we want to try to avoid situations where the button label text goes on two lines because it almost doubles the width of the buttons, like in the example you shared.

It was worthwhile testing the larger font size as part of the new M3 button changes, but I don't think we should make them the same size as the menu buttons. We originally increased the height as part of some of the other button updates because a user brought up that the touch area is sometimes difficult for larger fingers. Unfortunately, increasing the height and font size so it is unified creates other issues, with the button labels being too long and taking up too much space on smaller devices.

Proposal: To make the button and font size ratio feel more unified, I think we need to decrease the button height and font size back to what we had before.

We also wanted to explore using a lighter grey for the outlined button. The dark outline is problematic with the "Back" and "Next" buttons in particular, and it can be distracting from the form. Maybe something like #C4C4C4 would be slightly lighter but still visible.

@grzesiek2010
Copy link
Member Author

Proposal: To make the button and font size ratio feel more unified, I think we need to decrease the button height and font size back to what we had before.

Do you mean to what we have on master?

We also wanted to explore using a lighter grey for the outlined button. The dark outline is problematic with the "Back" and "Next" buttons in particular, and it can be distracting from the form. Maybe something like #C4C4C4 would be slightly lighter but still visible.

I'm not sure... wouldn't they look like disabled buttons then?
Screenshot_1697032564

@alyblenkin
Copy link
Collaborator

Do you mean to what we have on master?

Yes

I'm not sure... wouldn't they look like disabled buttons then?

Sorry for the slow reply! I don’t think they look like disabled buttons because of the blue text. However, #C4C4C4 doesn’t pass for accessibility in terms of contrast. I assume that a lot of companies, like Google, use a lighter outline but rely on text contrast.
I think a middle ground would likely be our best option at this point and go with a slightly darker grey #909090. And we don’t want the bold font because it’s very difficult to read.

I also don’t want this work to eat too much into the other features we have planned for this release. So I think we should go forward with the slightly softer version, and we can always change it as we navigate the wonderful world of implementing M3 styling :)

@grzesiek2010
Copy link
Member Author

What about the dark mode? What color should be used there?

@alyblenkin
Copy link
Collaborator

What about the dark mode? What color should be used there?

Dark mode is fine! We don't want to make any changes there.

@grzesiek2010 grzesiek2010 marked this pull request as ready for review October 17, 2023 08:42
@grzesiek2010
Copy link
Member Author

@alyblenkin according to https://github.com/material-components/material-components-android/blob/master/docs/components/Button.md#container-attributes-3 the strokeColor should be ?attr/colorOnSurface at 12% opacity in our case it's color_on_surface_low_emphasis I used it and I think it looks ok. What do you think?

@grzesiek2010 grzesiek2010 changed the title Button styling: increase font size Button styling improvements Oct 17, 2023
@alyblenkin
Copy link
Collaborator

@alyblenkin according to https://github.com/material-components/material-components-android/blob/master/docs/components/Button.md#container-attributes-3 the strokeColor should be ?attr/colorOnSurface at 12% opacity in our case it's color_on_surface_low_emphasis I used it and I think it looks ok. What do you think?

Ah perfect! I didn't know we had the Material colours implemented.

Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

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

Looks good! I've left a comment about one small tweak I'd like.

One thing I was thinking: did you consider introducing a theme for widgets that overrides the default button style instead of the more "programmatic" approach (with adjustFontSize)? It might be worth experimenting with. The end result would be the same (that buttons would all have a different default style from the rest of the app), but it would still allow us to have Button views in widgets that set their own text style which the current approach would not allow.

@grzesiek2010
Copy link
Member Author

One thing I was thinking: did you consider introducing a theme for widgets that overrides the default button style instead of the more "programmatic" approach (with adjustFontSize)?

adjustFontSize is used to set the font size dynamically based on the settings. A new them would be good if it was static right?

@seadowg
Copy link
Member

seadowg commented Oct 31, 2023

adjustFontSize is used to set the font size dynamically based on the settings. A new them would be good if it was static right?

Ah good point! We'd still need a programmatic part to update the theme which would probably be pretty messy.

@dbemke
Copy link

dbemke commented Nov 7, 2023

If there is Cancel button (e.g. in settings) there isn't any outline and the button looks different than the other buttons in the app. Does this PR apply to all of buttons in the app?
Cancelbutton

@grzesiek2010
Copy link
Member Author

Does this PR apply to all of buttons in the app?

Yes if it's a button with a border (the outline buttons with white background) then the color should be updated across the app. If there were buttons without borders it's not in the scope of this pr. Should they have borders I don't know I think it's ok as it is.

@dbemke
Copy link

dbemke commented Nov 7, 2023

The form map icon/button has a darker outline than other buttons. Is this in the scope of this PR?

@grzesiek2010
Copy link
Member Author

The form map icon/button has a darker outline than other buttons. Is this in the scope of this PR?

image

this one?

@dbemke
Copy link

dbemke commented Nov 7, 2023

Yes. If it's in the scope of the PR there are also buttons with icons in forms with media
buttonswithicons

@grzesiek2010
Copy link
Member Author

So yes those also should be updated. I've just fixed that so it should be fine now.

@alyblenkin
Copy link
Collaborator

Yes if it's a button with a border (the outline buttons with white background) then the color should be updated across the app. If there were buttons without borders it's not in the scope of this pr. Should they have borders I don't know I think it's ok as it is.

It's a really good point @dbemke and it has caused a lot of confusion in the UX world! Cancel buttons are navigation links, so they don't need an outline. For example:

Navigation links (without an outline)

  • Cancel
  • Do not delete
  • Close

You're not changing any information here, you are simply navigating back to the previous state.

Action buttons (with outline)

  • Confirm
  • OK
  • Download
  • Create

The key here is the action button is changing the data or manipulating something.

@dbemke
Copy link

dbemke commented Nov 8, 2023

Tested with Success!

Verified on device with Android 8.1, 10

Verified cases:

  • buttons in the dark and light mode in the app

@srujner
Copy link

srujner commented Nov 8, 2023

Tested with Success!

Verified on device with Android 13

@grzesiek2010 grzesiek2010 merged commit 6bd18c4 into getodk:master Nov 8, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button styling: increase font size
5 participants