-
Notifications
You must be signed in to change notification settings - Fork 990
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
Web string parity #31
Conversation
@@ -451,6 +452,15 @@ public boolean shouldIncludeFeatured() { | |||
} | |||
} | |||
|
|||
/** | |||
* Determines if params are for All Projects ("Everything"), i.e. discovery without params. |
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.
I don't have the latest iOS app compiling, but is the filter now 'All Projects' instead of 'Everything'?
If so, we could update the string on Android and remove references to 'Everything' throughout the source.
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.
yep, 'All Projects', forgot to mention that change up top
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.
In filterString
, do we need to change R.string.discovery_everything
to R.string.All_projects
or something?
@@ -214,7 +214,7 @@ public DiscoveryViewModel(final @NonNull Environment environment) { | |||
.take(1) | |||
.map(Intent::getAction) | |||
.filter(Intent.ACTION_MAIN::equals) | |||
.map(__ -> DiscoveryParams.builder().staffPicks(true).build()); | |||
.map(__ -> DiscoveryParams.builder().build()); |
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.
Are we sure we want to alter the functionality here? Seems like this is a behavioral change rather than just language?
I think it might also introduce a bug with the onboarding – https://github.com/kickstarter/android-oss/pull/31/files#diff-35cad2327f6b5b0f53fd5b9f64ee033bR278 onboarding is only visible when staff picks are true, but now we've turned them off by default?
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.
yes on altering functionality--on ios we now default to All Projects rather than PWL. good call with the onboarding bug! will address
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.
Okay. I'm not sure how crucial it is to the tests, but throughout DiscoveryFragmentViewModelTest
we set staff picks to true when loading the initial params, e.g.: https://github.com/kickstarter/android-oss/pull/31/files#diff-295515ebc9274113707ed3b807dd41dcR137. Seems like the new default would not have that param set to true.
Would recommend doing a pass throughout the codebase where staffPicks
is set to true and make sure we don't have any outdated assumptions about what the initial load looks like.
} else if (location() != null) { | ||
return location().displayableName(); | ||
} else if (recommended() != null && recommended()) { |
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.
@@ -43,7 +50,7 @@ public void bindData(final @Nullable Object data) throws Exception { | |||
public void onBind() { | |||
final Context context = context(); | |||
|
|||
filterTextView.setText(item.params().filterString(context)); | |||
filterTextView.setText(item.params().filterString(context, ksString, false, true)); |
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.
Instead of injecting ksString
in the constructor, you may also want to consider using environment().ksString()
.
Tracing that through:
- We cast the context to a
KSApplication
object here:return ((KSApplication) context().getApplicationContext()).component().environment(); KSApplication
's component gives us access to things injected fromApplicationModule
, including the environment: https://github.com/kickstarter/android-oss/blob/master/app/src/main/java/com/kickstarter/ApplicationModule.java#L95
We're already using environment
in view models, but we can use it elsewhere too. Eventually it'd be great to have Dagger build our dependency graph, but generally use environment
to access globals similar to iOS since it makes test injection pretty straightforward.
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.
This is a very good suggestion. It did feel a little dated to be injecting KSString
into all these view holders. Thanks!
what
Some language has updated since we've last released an android build. This PR makes the following changes:
Staff Picks -> Projects We Love
Friends Backed -> Following
Magic -> Home
Starred -> Saved
Everything -> All Projects
For starred and staff picks, only the displayed strings have changed and not the code logic, since we still use that terminology internally (e.g.
starred=1
).Recommended
project filter will come in a later pass.EDIT: Adding the
Recommended for you
filter turned out to be quite straightforward (9b6983e) so it has been added to this PR as well.todo
All Projects