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

(fixes #2872) Refactoring unused files for "showcase view whole nearby activity" #5870

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

devbytespark
Copy link

@devbytespark devbytespark commented Oct 19, 2024

Description (required)

The refactored showcase nearby activity previously relied on a hardcoded string located in this file. This resulted in some inconsistency between the two activities.

Fixes #2872


What changes did you make and why?

While the best approach would be to parameterize the string with the key showcase_view_whole_nearby_activity to ensure consistency, the activity that previously utilized that layout has been removed. Consequently, the initial approach with the following pseudo-code is no longer applicable:

Parameterise the showcase string

<string name="showcase_view_whole_nearby_activity">These are the places near you that need pictures to illustrate their Wikipedia articles.\n\nClicking on \'%1$s' locks the map and launches a nearby search around that location.</string>

<string name="search_this_area">Search this area</string>

Set the showcase string to use the search_this_area string

String searchString = getString(R.string.search_this_area);
String message = String.format(getString(R.string.showcase_view_whole_nearby_activity), searchString);

As a temporary workaround, I have edited the hardcoded string to ensure it aligns with the expected output from the search_this_area string and disabled the automatic capitalization flag in the button for consistency.


Tests performed (required)
Tested on ProdDebug on Pixel 9 with API level 34 to ensure consistency with the capitalization as shown in the following two screenshots.

Before the change:
before_change

After the change:
after_change

@@ -303,7 +303,7 @@

<string name="about_translate_cancel">Cancel</string>
<string name="retry">Retry</string>
<string name="showcase_view_whole_nearby_activity">These are the places near you that need pictures to illustrate their Wikipedia articles.\n\nClicking on \'SEARCH THIS AREA\' locks the map and launches a nearby search around that location.</string>
<string name="showcase_view_whole_nearby_activity">These are the places near you that need pictures to illustrate their Wikipedia articles.\n\nClicking on \'Search this area\' locks the map and launches a nearby search around that location.</string>
Copy link
Member

Choose a reason for hiding this comment

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

There is no "Search this area" button anymore.
Actually, is this string used somewhere? If you find where it is used, would you mind posting a screenshot?
Thanks a lot! 🙂

Copy link
Author

Choose a reason for hiding this comment

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

I found that it was used in dialog_nearby.xml, but I believe the activity using the layout has been refactored.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind trying to remove it? Any impact? 🙂

Copy link
Author

@devbytespark devbytespark Oct 19, 2024

Choose a reason for hiding this comment

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

Deleting the button and its string seems fine

Copy link
Author

@devbytespark devbytespark Oct 24, 2024

Choose a reason for hiding this comment

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

Hey Nicholas after further review I noticed that the dialog_nearby.xml file had no usages in the whole app. I’ve safely deleted the file along with its associated string resources, and the project still builds without issues.

It seems the file was part of the old webview in the nearby section of the contributions activity, likely from a version of the app prior to refactoring.

app/src/main/res/layout/dialog_nearby.xml Outdated Show resolved Hide resolved
@@ -62,6 +62,7 @@
android:padding="@dimen/activity_margin_horizontal"
android:singleLine="true"
android:text="@string/search_this_area"
android:textAllCaps="false"
Copy link
Member

Choose a reason for hiding this comment

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

What is the problem if you don't add this line?

Copy link
Author

Choose a reason for hiding this comment

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

The line was initially included to ensure that the button text aligned with the original TextView string (which has now been removed) as Android Studio defaults buttons to AllCaps. However, since we are removing it, it may no longer be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

How about removing this whole button?

Copy link
Author

Choose a reason for hiding this comment

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

While the TextView that used to prompt users to use this button has been refactored and removed, the button remains in the app and is still used in the Map Explore activity, as shown below button_usage

Copy link
Member

Choose a reason for hiding this comment

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

Do you know what makes the text become uppercase in the first place?
We should fix the root issue rather than address this single symptom.
Would you mind checking whether these are really needed?

app/src//main/res/layout-land/activity_review.xml:        android:textAllCaps="true"
app/src//main/res/layout/bottom_sheet_details_explore.xml:            android:textAllCaps="true"
app/src//main/res/layout/bottom_sheet_details_explore.xml:            android:textAllCaps="true"
app/src//main/res/layout/bottom_sheet_details_explore.xml:          android:textAllCaps="true"
app/src//main/res/layout/fragment_achievements.xml:            android:textAllCaps="true"/>
app/src//main/res/layout/fragment_achievements.xml:            android:textAllCaps="true"/>
app/src//main/res/layout/custom_nearby_tab_layout.xml:        android:textAllCaps="true"
app/src//main/res/layout/activity_review.xml:      android:textAllCaps="true"
app/src//main/res/layout/fragment_review_image.xml:      android:textAllCaps="true"
app/src//main/res/layout/fragment_review_image.xml:      android:textAllCaps="true"

@nicolas-raoul

This comment was marked as off-topic.

@nicolas-raoul

This comment was marked as off-topic.

@devbytespark devbytespark changed the title (fixes #2872) Capitalization consistency for "showcase view whole nearby activity" (fixes #2872) Refactoring unused files for "showcase view whole nearby activity" Oct 24, 2024
@@ -62,6 +62,7 @@
android:padding="@dimen/activity_margin_horizontal"
android:singleLine="true"
android:text="@string/search_this_area"
android:textAllCaps="false"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what makes the text become uppercase in the first place?
We should fix the root issue rather than address this single symptom.
Would you mind checking whether these are really needed?

app/src//main/res/layout-land/activity_review.xml:        android:textAllCaps="true"
app/src//main/res/layout/bottom_sheet_details_explore.xml:            android:textAllCaps="true"
app/src//main/res/layout/bottom_sheet_details_explore.xml:            android:textAllCaps="true"
app/src//main/res/layout/bottom_sheet_details_explore.xml:          android:textAllCaps="true"
app/src//main/res/layout/fragment_achievements.xml:            android:textAllCaps="true"/>
app/src//main/res/layout/fragment_achievements.xml:            android:textAllCaps="true"/>
app/src//main/res/layout/custom_nearby_tab_layout.xml:        android:textAllCaps="true"
app/src//main/res/layout/activity_review.xml:      android:textAllCaps="true"
app/src//main/res/layout/fragment_review_image.xml:      android:textAllCaps="true"
app/src//main/res/layout/fragment_review_image.xml:      android:textAllCaps="true"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggested fixes for "showcase view whole nearby activity"
2 participants