-
Notifications
You must be signed in to change notification settings - Fork 539
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
Fix part of #5661: Add Missing style Attributes to TextViews #5682
base: develop
Are you sure you want to change the base?
Conversation
- Applied the new styles in the respective layout XML files - Removed 2 TextView attribute IDs from the exception list in TextViewStyleCheck.kt
am i heading right? @manas-yu |
@@ -146,4 +143,4 @@ | |||
app:layout_constraintTop_toTopOf="parent" /> | |||
</androidx.constraintlayout.widget.ConstraintLayout> | |||
</ScrollView> | |||
</layout> | |||
</layout> |
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.
Always add an extra line at the end of the file
Hi @Harsh2118-hub , Ensure that all
You can even confirm after removing the IDs if any reference is left out by running the command: |
@@ -826,4 +826,17 @@ | |||
<item name="endIconDrawable">@drawable/ic_arrow_drop_down_black_24dp</item> | |||
<item name="endIconTint">@color/component_color_shared_black_background_color</item> | |||
</style> | |||
|
|||
<style name="WalkthroughFinalNoTextViewStyle" parent="TextView.Common1"> |
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.
You can reuse these styles for the respective IDs present in layout-land/walkthrough_final_fragment.xml
@manas-yu thanks for guidance |
please take a look @manas-yu , this time the command- bazel run //scripts:check_textview_styles -- $(pwd) is working |
Unassigning @Harsh2118-hub since a re-review was requested. @Harsh2118-hub, please make sure you have addressed all review comments. Thanks! |
@@ -189,14 +189,6 @@ private class TextViewStyleCheck { | |||
|
|||
// TODO(#5661): Add missing styles for TextView IDs. | |||
private val attributeIds = listOf( |
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.
Ah looks like this was overlooked in #5599 the attributeIds
has repeated IDs and could cause an issue it would be better to replace it with the below :
Set of ids
private val attributeIds = listOf(
"@+id/developer_options_text_view",
"@+id/onboarding_language_text_view",
"@+id/walkthrough_final_no_text_view",
"@+id/walkthrough_final_yes_text_view",
"@+id/walkthrough_final_title_text_view",
"@+id/chapter_index",
"@+id/test_text_view",
"@+id/feedback_text_view",
"@+id/item_selection_contents_text_view",
"@+id/learner_analytics_sync_status_text_view",
"@+id/text_view_for_int_no_data_binding",
"@+id/walkthrough_topic_name_text_view",
"@+id/walkthrough_lesson_count_text_view",
"@+id/hint_bar_title",
"@+id/coming_soon_text_view",
"@+id/topic_name_text_view",
"@+id/lesson_count_text_view",
"@+id/multiple_choice_content_text_view",
"@+id/language_text_view",
"@+id/welcome_text_view",
"@+id/app_version_text_view",
"@+id/app_last_update_date_text_view",
"@+id/test_margin_text_view",
"@+id/content_text_view",
"@+id/action_options",
"@+id/action_help",
"@+id/action_close",
"@+id/continue_studying_text_view",
"@+id/language_unavailable_notice",
"@+id/story_progress_chapter_completed_text",
"@+id/profile_id_view_profile_name",
"@+id/profile_id_view_learner_id",
"@+id/learner_events_waiting_upload_label",
"@+id/learner_events_waiting_upload_count",
"@+id/learner_events_uploaded_label",
"@+id/learner_events_uploaded_count",
"@+id/uncategorized_events_waiting_upload_label",
"@+id/uncategorized_events_waiting_upload_count",
"@+id/uncategorized_events_uploaded_label",
"@+id/uncategorized_events_uploaded_count",
"@+id/text_view_for_live_data_no_data_binding",
"@+id/selection_interaction_textview",
"@+id/onboarding_steps_count",
"@+id/profile_picture_edit_dialog_view_picture",
"@+id/profile_picture_edit_dialog_change_picture",
"@+id/chapter_title",
"@+id/walkthrough_welcome_title_text_view",
"@+id/story_name_text_view",
"@+id/copyright_license_text_view",
"@+id/ga_update_notice_dialog_message",
"@+id/create_profile_picture_prompt",
"@+id/profile_reset_pin_main",
"@+id/submitted_answer_text_view",
"@+id/end_session_header_text_view",
"@+id/end_session_body_text_view",
"@+id/question_progress_text",
"@+id/congratulations_text_view",
"@+id/beta_notice_dialog_message",
"@+id/onboarding_language_explanation",
"@+id/create_profile_title",
"@+id/profile_name_text_view",
"@+id/resume_lesson_chapter_title_text_view",
"@+id/story_count_text_view",
"@+id/download_size_text_view",
"@+id/options_activity_selected_options_title",
"@+id/profile_select_text",
"@+id/extra_controls_title",
"@+id/view_all_text_view"
)
I think we can handle this change in this PR only as opening a separate one for this issue doesn't seem necessary. Please replace the current attributeIds
with the one I provided above after removing the IDs you have already added the style attribute for.
</layout> | ||
style="@style/TestTextViewStyle" | ||
android:text="@string/app_name" /> | ||
</layout> |
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.
Add an extra line at the end of the file, same for the rest.
Thanks @Harsh2118-hub for the changes and PTAL at this comment and re-run the command. The only reference left is the Also add to the PR description for replacing the list of |
@manas-yu PTAL , can i move forward with remaning attributes id which are left . |
Unassigning @Harsh2118-hub since a re-review was requested. @Harsh2118-hub, please make sure you have addressed all review comments. Thanks! |
Thanks @Harsh2118-hub for the changes! You can work on three more IDs to complete the PR as mentioned here, and it will be good to go. Also please update the PR description and title as I mentioned earlier |
Thanks, @Harsh2118-hub. This looks good. Just update the PR title and description with "Fix part of #5661" and resolve the left out comment. |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 3636 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 367 bytes (Added) Method count: 260299 (old), 260299 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6830 (old), 6840 (new), 10 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 3596 bytes (Added)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 1940 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1505 bytes (Added) Method count: 115822 (old), 115822 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5799 (new), 1 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 1908 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 2180 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 28 bytes (Removed) Method count: 115828 (old), 115828 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5799 (new), 1 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 2140 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 2144 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 17 bytes (Added) Method count: 115828 (old), 115828 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5799 (new), 1 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 2108 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 3640 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 324 bytes (Added) Method count: 260299 (old), 260299 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6830 (old), 6840 (new), 10 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 3604 bytes (Added)
Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 1928 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1459 bytes (Added) Method count: 115822 (old), 115822 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5799 (new), 1 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 1892 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 2168 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 15 bytes (Removed) Method count: 115828 (old), 115828 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5799 (new), 1 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 2132 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 2140 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 67 bytes (Added) Method count: 115828 (old), 115828 (new), 0 (No change) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5798 (old), 5799 (new), 1 (Added)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 10 MiB (old), 10 MiB (new), 2108 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
PTAL @adhiamboperes |
Hi @manas-yu , I wanted to follow up on this PR and check if there are any updates. Please let me know if there's anything I can do to assist or anything I can do to expedite the review. |
"Fix part of #5661"
-Edited attributeIds list in TextViewStyleCheck which don't has repeated IDs and id's for those text view and style.xml file has been updated.
-edited list
private val attributeIds = listOf(
"@+id/text_view_for_int_no_data_binding",
"@+id/walkthrough_topic_name_text_view",
"@+id/walkthrough_lesson_count_text_view",
"@+id/hint_bar_title",
"@+id/coming_soon_text_view",
"@+id/topic_name_text_view",
"@+id/lesson_count_text_view",
"@+id/multiple_choice_content_text_view",
"@+id/language_text_view",
"@+id/welcome_text_view",
"@+id/app_version_text_view",
"@+id/app_last_update_date_text_view",
"@+id/test_margin_text_view",
"@+id/content_text_view",
"@+id/action_options",
"@+id/action_help",
"@+id/action_close",
"@+id/continue_studying_text_view",
"@+id/language_unavailable_notice",
"@+id/story_progress_chapter_completed_text",
"@+id/profile_id_view_profile_name",
"@+id/profile_id_view_learner_id",
"@+id/learner_events_waiting_upload_label",
"@+id/learner_events_waiting_upload_count",
"@+id/learner_events_uploaded_label",
"@+id/learner_events_uploaded_count",
"@+id/uncategorized_events_waiting_upload_label",
"@+id/uncategorized_events_waiting_upload_count",
"@+id/uncategorized_events_uploaded_label",
"@+id/uncategorized_events_uploaded_count",
"@+id/text_view_for_live_data_no_data_binding",
"@+id/selection_interaction_textview",
"@+id/onboarding_steps_count",
"@+id/profile_picture_edit_dialog_view_picture",
"@+id/profile_picture_edit_dialog_change_picture",
"@+id/chapter_title",
"@+id/walkthrough_welcome_title_text_view",
"@+id/story_name_text_view",
"@+id/copyright_license_text_view",
"@+id/ga_update_notice_dialog_message",
"@+id/create_profile_picture_prompt",
"@+id/profile_reset_pin_main",
"@+id/submitted_answer_text_view",
"@+id/end_session_header_text_view",
"@+id/end_session_body_text_view",
"@+id/question_progress_text",
"@+id/congratulations_text_view",
"@+id/beta_notice_dialog_message",
"@+id/onboarding_language_explanation",
"@+id/create_profile_title",
"@+id/profile_name_text_view",
"@+id/resume_lesson_chapter_title_text_view",
"@+id/story_count_text_view",
"@+id/download_size_text_view",
"@+id/options_activity_selected_options_title",
"@+id/profile_select_text",
"@+id/extra_controls_title",
"@+id/view_all_text_view"
)
Applied the new styles in the respective layout XML files
Removed TextView attribute IDs from the exception list in TextViewStyleCheck.kt
-Created a new styles in styles.xml to define text appearance consistently.
-Updated textview to apply the newly created style .
from the exception list in TextViewStyleCheck.kt since it now follows the required style guidelines.
Essential Checklist