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

refractor: Replace all hard-coded string messages #1272

Merged
merged 2 commits into from
Apr 4, 2019
Merged

refractor: Replace all hard-coded string messages #1272

merged 2 commits into from
Apr 4, 2019

Conversation

anhanh11001
Copy link
Contributor

@anhanh11001 anhanh11001 commented Mar 11, 2019

Details:

  • Resolve merge conflicts
  • Some Bengali strings are not translated as I don't know how to do that, so I put "need translations"
  • Strings are extracted using Resource class

Fixes #1256

nikit19
nikit19 previously approved these changes Mar 12, 2019
@fossasia fossasia deleted a comment Mar 13, 2019
@iamareebjamal
Copy link
Member

Please resolve conflicts

@anhanh11001
Copy link
Contributor Author

@iamareebjamal I have resolved merge conflicts, please take a look

Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

No Android class should be used in ViewModel, including context

@fossasia fossasia deleted a comment Mar 24, 2019
@fossasia fossasia deleted a comment Mar 25, 2019
@anhanh11001
Copy link
Contributor Author

@iamareebjamal please take a look, I have updated another solution on extracting resource in ViewModel

@@ -24,7 +26,7 @@ class AboutEventViewModel(private val eventService: EventService) : ViewModel()

fun loadEvent(id: Long) {
if (id.equals(-1)) {
mutableError.value = "Error fetching event"
mutableError.value = OpenEventGeneral.appResource.getString(R.string.error_fetching_event_message)
Copy link
Member

Choose a reason for hiding this comment

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

This is still using resources in viewmodel

Easy way to check if you are using resources in ViewModel - Instantiate the ViewModel in a test and run all methods

If the test crashes, you are using Android class in ViewModel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that now and referencing string resource inside ViewModel is not safe, so I don't find a way to extract those strings inside ViewModel.

And I have read the Android documentation, and it said "if the ViewModel needs the Application context, it can extend the AndroidViewModel class and have a constructor that receives the Application in the constructor". Should I use this technique to extract the hard-coded strings inside the view model into res/values/strings file or I should leave them unextracted?

Copy link
Member

Choose a reason for hiding this comment

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

@anhanh11001 anhanh11001 changed the title fix: Replace all hard-coded string messages refractor: Replace all hard-coded string messages Mar 27, 2019
@fossasia fossasia deleted a comment Mar 29, 2019
Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

Use dependency injection

No need to create an object for using each time

month.add(Resource().getString(R.string.september) ?: "September")
month.add(Resource().getString(R.string.october) ?: "October")
month.add(Resource().getString(R.string.november) ?: "November")
month.add(Resource().getString(R.string.december) ?: "December")
Copy link
Member

Choose a reason for hiding this comment

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

Redundant

<string name="postal_code">Needs translation</string>
<string name="payment_info">Needs translation</string>
<string name="removed_from_liked">Needs translation</string>
<string name="undo">Needs translation</string>
Copy link
Member

Choose a reason for hiding this comment

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

Remove all of these

Details:
- Resolve merge conflicts
- Strings are extracted using Resource class

Fixes: #1256
@fossasia fossasia deleted a comment Apr 2, 2019
@iamareebjamal iamareebjamal added this to the v0.2.0 milestone Apr 2, 2019
iamareebjamal
iamareebjamal previously approved these changes Apr 2, 2019
@iamareebjamal
Copy link
Member

All approved PRs beyond this point will be merged after release

@iamareebjamal iamareebjamal merged commit 8580868 into fossasia:development Apr 4, 2019
@anhanh11001 anhanh11001 deleted the 1256_hard_coded_strings branch June 20, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants