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

Fix #5136: Modifies all caps buttons to sentence case #5157

Merged
merged 8 commits into from
Sep 30, 2023

Conversation

theMr17
Copy link
Collaborator

@theMr17 theMr17 commented Sep 23, 2023

Explanation

Fixes #5136

This PR changes all caps buttons to sentence case. All buttons mentioned in this comment are changed either by directly changing its android:textAllCaps to false in the layout files or by changing its style in the styles.xml file.

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

Screenshots

layout

Screen / Dialog Button ID Screenshots
activity_input_interaction_view_test.xml submit_button
add_profile_activity.xml add_profile_activity_create_button
admin_auth_activity.xml admin_auth_submit_button
admin_pin_activity.xml submit_button
concept_card_fragment_test_activity.xml open_dialog_0 open_dialog_1
exploration_test_activity.xml play_exploration_button
hint_summary.xml reveal_hint_button image
math_expression_parser_fragment.xml parse_math_expression_button image
onboarding_slide_final.xml get_started_button image
profile_list_control_buttons.xml learner_analytics_share_ids_and_events_button learner_analytics_upload_logs_now_button
profile_rename_fragment.xml profile_rename_save_button image
profile_reset_pin_fragment.xml profile_reset_save_button
replay_button_item.xml replay_button
return_to_lesson_button_item.xml return_to_lesson_button image
return_to_topic_button_item.xml return_to_topic_button
solution_summary.xml show_solution_button
state_fragment_test_activity.xml play_test_exploration_button
submit_button_item.xml submit_answer_button image
topic_practice_footer_view.xml topic_practice_start_button
walkthrough_welcome_fragment.xml walkthrough_welcome_next_button

layout-land

Screen / Dialog Button ID Screenshots
onboarding_slide_final.xml get_started_button image
walkthrough_welcome_fragment.xml walkthrough_welcome_next_button

layout-sw600dp-land

Screen / Dialog Button ID
onboarding_slide_final.xml get_started_button

layout-sw600dp-port

Screen / Dialog Button ID
onboarding_slide_final.xml get_started_button

@theMr17 theMr17 requested a review from a team as a code owner September 23, 2023 08:31
@adhiamboperes adhiamboperes self-assigned this Sep 23, 2023
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @the-mr17!

  • Could you please verify that all buttons are in sentense case? To do that, you need to add some screenshots to this PR. See this PR for example, and the linked PR also contains some sample dialogs that need to be edited too.
  • Per ypur CI results, testStateFragment_nextState_viewShowSolutionDialog_clickCancel_canViewShowSolution is failing. Looks like the updated text case does not match the expected text in the test, so you need to update the test. See wiki for help inetpreting CI results.
  • Some buttons are hidden behind platform parameters, and to make sure you don't miss any, please perform a blanket search for "button"(CTRL+SHIFT+F).

@oppiabot
Copy link

oppiabot bot commented Sep 24, 2023

Unassigning @adhiamboperes since the review is done.

@oppiabot
Copy link

oppiabot bot commented Sep 24, 2023

Hi @the-mr17, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks!

@theMr17
Copy link
Collaborator Author

theMr17 commented Sep 24, 2023

  • Could you please verify that all buttons are in sentense case? To do that, you need to add some screenshots to this PR. See this PR for example, and the linked PR also contains some sample dialogs that need to be edited too.

Thanks @adhiamboperes, I am looking into it. But, I cannot find the linked PR.

@adhiamboperes
Copy link
Collaborator

  • Could you please verify that all buttons are in sentense case? To do that, you need to add some screenshots to this PR. See this PR for example, and the linked PR also contains some sample dialogs that need to be edited too.

Thanks @adhiamboperes, I am looking into it. But, I cannot find the linked PR.

Sorry, please see : #4946

@theMr17
Copy link
Collaborator Author

theMr17 commented Sep 24, 2023

  • Could you please verify that all buttons are in sentense case? To do that, you need to add some screenshots to this PR. See this PR for example, and the linked PR also contains some sample dialogs that need to be edited too.

@adhiamboperes Do I need to edit those dialog buttons in PR #4946 ? (for eg, OK, CANCEL, etc.). According to the figma layouts those all dialog buttons are in all caps.

@adhiamboperes
Copy link
Collaborator

  • Could you please verify that all buttons are in sentense case? To do that, you need to add some screenshots to this PR. See this PR for example, and the linked PR also contains some sample dialogs that need to be edited too.

@adhiamboperes Do I need to edit those dialog buttons in PR #4946 ? (for eg, OK, CANCEL, etc.). According to the figma layouts those all dialog buttons are in all caps.

@jlevick, could you please weigh in here?

@jlevick
Copy link

jlevick commented Sep 25, 2023

@adhiamboperes @the-mr17 these should not be in all-caps. They should be sentence case: Ok, Cancel, etc.
I thought the design system was changed to reflect this but I see it has not been. I'll ask the Android designers to update it. Thank you for reaching out and confirming the changes!

@theMr17
Copy link
Collaborator Author

theMr17 commented Sep 26, 2023

Hi @adhiamboperes, I have done the following changes, please have a look.

  • Could you please verify that all buttons are in sentense case? To do that, you need to add some screenshots to this PR. See this PR for example, and the linked PR also contains some sample dialogs that need to be edited too.

I have changed the dialog buttons on PR #4946 to sentence case. I have also added some screenshots on the PR message above.

  • Per ypur CI results, testStateFragment_nextState_viewShowSolutionDialog_clickCancel_canViewShowSolution is failing. Looks like the updated text case does not match the expected text in the test, so you need to update the test. See wiki for help inetpreting CI results.

I have updated the test to match the updated text case with the expected text case in the test.

  • Some buttons are hidden behind platform parameters, and to make sure you don't miss any, please perform a blanket search for "button"(CTRL+SHIFT+F).

I performed a blanket search for "button" but did not find anything that needs to be changed related to this.

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

I have found from your screenshots that profile_rename_save_button is in all lowercase instead of sentence case.
The exit profile dialog(nav drawer -> switch profile) is still in all caps. So are the start over and continue lesson buttons in the resume lesson fragment. audio_language_select_dialog_okay_button is also still in all caps.

Screenshot_1695923309 Screenshot_1695923581 Screenshot_1695924139

@oppiabot
Copy link

oppiabot bot commented Sep 28, 2023

Unassigning @adhiamboperes since the review is done.

@oppiabot
Copy link

oppiabot bot commented Sep 28, 2023

Hi @the-mr17, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks!

@theMr17
Copy link
Collaborator Author

theMr17 commented Sep 29, 2023

@adhiamboperes I have done the above mentioned changes. Here are screenshots for the same.

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @the-mr17. LGTM.

@oppiabot oppiabot bot added the PR: LGTM label Sep 29, 2023
@oppiabot
Copy link

oppiabot bot commented Sep 29, 2023

Hi @the-mr17, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

@adhiamboperes adhiamboperes enabled auto-merge (squash) September 29, 2023 11:10
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

@the-mr17 could you please fix the failing tests? See https://github.com/oppia/oppia-android/wiki/Interpreting-CI-Results

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Thanks @the-mr17. This LGTM.

@oppiabot oppiabot bot added the PR: LGTM label Sep 30, 2023
@oppiabot
Copy link

oppiabot bot commented Sep 30, 2023

Hi @the-mr17, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

@adhiamboperes adhiamboperes enabled auto-merge (squash) September 30, 2023 02:24
@adhiamboperes adhiamboperes merged commit 9bbf94a into oppia:develop Sep 30, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Remove all-caps for all buttons on Android app and change to sentence case
3 participants