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

All warnings in "Usability:Typography", "Usability:Icons", and "Usability" categories #5194

Closed
Tracked by #5169
adhiamboperes opened this issue Oct 17, 2023 · 9 comments · Fixed by #5196
Closed
Tracked by #5169
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks.

Comments

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Oct 17, 2023

Description

We would like to fix all the remaining lint issues on Oppia Android in the "Usability:Typography", "Usability:Icons", and "Usability" categories.

Steps To Reproduce

Run the following command from a terminal in a checked-out Oppia Android repo:

./gradlew :app:lint

Expected Behavior

Running the command above should not show any errors in this category

@adhiamboperes
Copy link
Collaborator Author

adhiamboperes commented Oct 17, 2023

@Rd4dev, per #5169 (comment), could you please list some of the borderless buttons that need to be changed? We can consider creating a theme or a style that is applied globally.

For text capitalization, we use sentence case in buttons, not lowercase or uppercase.

@adhiamboperes
Copy link
Collaborator Author

adhiamboperes commented Oct 17, 2023

@BenHenning, do we want to create per hdpi image assets for this repository? see commit: Rd4dev@f6b21fe? If not, we need to supress this lint check.

I am also concerned that replacing ... and - instances in Arabic translations with unicode may make it difficult for translators to understand the strings.

@adhiamboperes adhiamboperes added good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks. bug End user-perceivable behaviors which are not desirable. labels Oct 17, 2023
@Rd4dev
Copy link
Collaborator

Rd4dev commented Oct 17, 2023

@adhiamboperes The warning about the borderless button emerged in the XML File -> concept_card_fragment_test_activity.xml, for the two buttons with IDs open_dialog_0 and open_dialog_1. If recommended, I can work on a separate theme or style for these buttons and incorporate them instead.

The lint warning regarding capitalization suggested that the standard Android convention for "Ok" is "OK" (tip: use '@android:string/ok' instead). In accordance with the sentence case used in oppia-android's coding practices, I can suppress this warning by using the following code snippet:
<string name="audio_language_select_dialog_okay_button" tools:ignore="ButtonCase,Typos">Ok</string>

@adhiamboperes
Copy link
Collaborator Author

Since the buttons here are test class buttons, your original proposed approach should be okay...no need to theme them.

@BenHenning
Copy link
Sponsor Member

@BenHenning, do we want to create per hdpi image assets for this repository? Rd4dev@f6b21fe? If not, we need to supress this lint check.

I am also concerned that replacing ... and - instances in Arabic translations with unicode may make it difficult for translators to understand the strings.

HDPI are fine to add for everything except our baked-in lesson thumbnails (mostly since those are going to go away as soon as we can remove them), but aside from that we may need to suppress the warning until such time that we can remove those images. Perhaps we can file a follow-up issue to do that work once it's understood what's involved?

Re: the ... and - issues, what exactly is the warning that Android Lint is outputting?

@Rd4dev
Copy link
Collaborator

Rd4dev commented Oct 18, 2023

@BenHenning
About the unicode warnings,
The warnings were under the Typography category claiming:

  • Ellipsis string can be replaced with ellipsis character
    Replace "..." with ellipsis character(..., …)?
    Link: strings.xml
  • Hyphen can be replaced with dash
    Replace "-" with an "en dash" character(-, –)?
    Link: strings.xml

@adhiamboperes adhiamboperes assigned BenHenning and Rd4dev and unassigned Rd4dev Oct 24, 2023
@adhiamboperes
Copy link
Collaborator Author

Assigning @BenHenning for follow up.

@BenHenning
Copy link
Sponsor Member

Sorry, missed this. Thanks @Rd4dev that helps. I think we can go ahead and make the suggested Unicode changes. Those characters should show up correctly in Translatewiki, and thus hopefully not confuse our translators.

Just to check: @adhiamboperes why were you specifically concerned for Arabic translations?

@BenHenning BenHenning removed their assignment Nov 17, 2023
@adhiamboperes
Copy link
Collaborator Author

@BenHenning, my concern was that the unicode may not show up correctly on translatewiki. Per your comment, this may not be a concern.

BenHenning added a commit that referenced this issue Jan 9, 2024
…sability" categories (#5196)

<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->
Fixes #5194 [part of #5182]

Fixes all warning in "Usability" category caused by

- **Borderless Buttons :** Fixed by introducing
`style="?android:attr/buttonBarButtonStyle"` to the borderless buttons
- **OK text capitalization :** Fixed by suppressing the lint warnings
for `ButtonCase` and `Typos`
- **Ellipsis/Hyphen unicode replacement :** Fixed by introducing unicode
representations (under review)
- **Image in density drawable folder :** Fixed by creating seperate
density images (under review)
- **Text size too small :** Fixed by upscaling text size to `12sp`
- **To incorporate Autofill :** Fixed by incorporating `autofillHints`
to the EditText

## ScreenShot

![Before After Usability Lint
Issues](https://github.com/oppia/oppia-android/assets/122200035/5a7037f5-ca22-482a-a0f3-8af2bfc8b75c)

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Co-authored-by: Ben Henning <ben@oppia.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Work: Low Solution is clear and broken into good-first-issue-sized chunks.
3 participants