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

Add CI check to ensure all TextViews have a style check #3972

Open
BenHenning opened this issue Oct 26, 2021 · 8 comments · May be fixed by #5599
Open

Add CI check to ensure all TextViews have a style check #3972

BenHenning opened this issue Oct 26, 2021 · 8 comments · May be fixed by #5599
Assignees
Labels
enhancement End user-perceivable enhancements. 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: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@BenHenning
Copy link
Member

Related issue: #3971. We need to make sure that TextViews have a centrally managed style set so that RTL/LTR settings can be properly managed. To do this, we need a CI check that runs a script which parses all layout files to ensure that TextView elements have a style set that corresponds to a local codebase style (i.e. uses @style/).

For reference, I suggest looking at the manifest transformation script since it parses & modifies XML today (so it's a good basis for the parsing needs of this new TextView script).

@BenHenning
Copy link
Member Author

/cc @veena14cs

@yash10019coder
Copy link
Contributor

@BenHenning I would like to work on this issue

@BenHenning
Copy link
Member Author

BenHenning commented Jan 5, 2022

@BenHenning I would like to work on this issue

Sounds good! Assigning this to you @yash10019coder.

@Broppia Broppia added the Impact: Low Low perceived user impact (e.g. edge cases). label Jun 13, 2022
@BenHenning BenHenning added Z-ibt Temporary label for Ben to keep track of issues he's triaged. issue_user_developer labels Sep 15, 2022
@seanlip seanlip added enhancement End user-perceivable enhancements. and removed issue_type_infrastructure labels Mar 28, 2023
@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
@adhiamboperes adhiamboperes added the good first issue This item is good for new contributors to make their pull request. label Mar 26, 2024
@Rd4dev Rd4dev self-assigned this Apr 12, 2024
@Rd4dev
Copy link
Collaborator

Rd4dev commented Jul 23, 2024

Unassigning myself to free it up for others. Will pick it up later if it's still available.

@Rd4dev Rd4dev removed their assignment Jul 23, 2024
@manas-yu
Copy link
Collaborator

manas-yu commented Dec 6, 2024

HI @adhiamboperes can i work on this issue, i'll add the requested TextViewStyleChecker script.

@adhiamboperes
Copy link
Collaborator

@manas-yu, could you please provide an outline of how you would approach this issue?

@manas-yu
Copy link
Collaborator

manas-yu commented Dec 10, 2024

@manas-yu, could you please provide an outline of how you would approach this issue?

@adhiamboperes I'll add the TextViewStyleChecker in scripts/xml, using TransformAndroidManifest/XmlSyntaxCheck.kt as a reference. The script will verify whether TextView elements are missing the @style attribute. I'll update the BUILD.bazel file to include this new script and add corresponding tests to ensure it works as expected.

@manas-yu manas-yu linked a pull request Dec 16, 2024 that will close this issue
6 tasks
@adhiamboperes
Copy link
Collaborator

@manas-yu, re #5599 (comment)

I need some clarification regarding the issue. The issue states that the script should parse all layout XML (files in app/src/main/res/layout*) and ensure all TextView elements have a style set (using @style/) that aligns with the local codebase style. However, some files (mentioned in the previous comment) have TextView elements without a style attribute. Certain cases need to be considered, such as when the text is center-aligned (gravity="center"), uses hardcoded left/right attributes, or lacks direction-sensitive layout attributes. To proceed, I would need an exact set of criteria to differentiate TextView elements with a style set for RTL/LTR settings from those without. This will help ensure the script enforces the style requirement accurately without affecting valid exceptions.

  • Each TextView must include a style attribute referencing a centrally managed style (e.g., @style/YourTextViewStyle).
  • The centrally managed style should handle RTL/LTR settings by specifying textAlignment, gravity, and any other properties that affect directionality.
  • If the TextView has gravity="center", it may not require a style explicitly managing directionality since it is inherently direction-neutral.
  • Avoid enforcing the style check for elements with hardcoded left or right attributes (e.g., paddingLeft, layout_marginRight), but log these cases for potential future refactoring to use start/end attributes instead.
  • TextViews with no text or direction-sensitive layout attributes (e.g., purely used for spacing or decoration) may also be valid without a style, though these should be carefully reviewed to confirm.

I would defer to @BenHenning for further clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. 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: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Projects
8 participants