-
Notifications
You must be signed in to change notification settings - Fork 11
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
ALTAPPS-506: Shared load gems and streak on the Track screen #315
Merged
ivan-magda
merged 21 commits into
develop
from
feature/ALTAPPS_506/shared_track_gems_streak
Jan 16, 2023
Merged
ALTAPPS-506: Shared load gems and streak on the Track screen #315
ivan-magda
merged 21 commits into
develop
from
feature/ALTAPPS_506/shared_track_gems_streak
Jan 16, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deletes unnecessary load of all pages.
GamificationToolbarFeature - Nice naming |
XanderZhu
reviewed
Jan 14, 2023
shared/src/commonMain/kotlin/org/hyperskill/app/track/injection/TrackComponentImpl.kt
Outdated
Show resolved
Hide resolved
XanderZhu
reviewed
Jan 14, 2023
...kotlin/org/hyperskill/app/gamification_toolbar/injection/GamificationToolbarComponentImpl.kt
Show resolved
Hide resolved
XanderZhu
reviewed
Jan 14, 2023
shared/src/commonMain/kotlin/org/hyperskill/app/home/presentation/HomeFeature.kt
Show resolved
Hide resolved
I think it would be useful to follow this approach. |
XanderZhu
added
awaiting changes
Changes requested, waiting on author to update
and removed
awaiting review
Pull Request is awaiting code reviews
labels
Jan 14, 2023
XanderZhu
added
ready to pull
Pull Request is ready to merge
and removed
awaiting changes
Changes requested, waiting on author to update
labels
Jan 16, 2023
XanderZhu
approved these changes
Jan 16, 2023
To not contact with API on Android on Track screen.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
YouTrack Issues:
#ALTAPPS-506, #ALTAPPS-518
Checklist
Before Code Review:
Description
Miscellaneous changes:
StreaksDataComponent
that providesStreaksInteractor
. It helps to avoid code duplication on the injection level.StreaksRemoteDataSourceImpl
, no need to load all pages, because we are using only the firstStreak
element.getRepetitionsState
into helper function inHomeActionDispatcher
, which makes code more readable.forceUpdate
value parameter forHomeFeature.Message.Initialize
message ontryAgain
inHomeFragment
Primary changes:
GamificationToolbarFeature
that encapsulates loading of thestreak
and hypercoins balance. It also moves analytic events to this feature andViewAction
s.HomeFeature
to using theGamificationToolbarFeature
on iOS and Android.TrackFeature
with theGamificationToolbarFeature
on iOS only. Android implementation has remote data loading, and messages that trigger remote data loading are marked withTODO
, we should implement UI or delete those messages before merging this PR.Also, we could send messages to the
GamificationToolbarFeature
from the parent feature directly. For exampleMessage.Initialize
. The current implementation sends 2 messages: one for the parent feature (home or track) and the second for the toolbar:we could update our
HomeReducer
to initialize home and toolbar in one place:I tried this approach in the
HomeReducer
and it's become harder to read/understand code and more complicated than just sending messages from the Fragment on Android or ViewModel on iOS.I think there is no silver bullet - the above approach adds the ability to send only one message to initialize parent and child features, but the source code gets more complicated in the reducer and the other approach with sending distinct messages (first for home or track and second for toolbar) leaves reducer simpler, but we could forget to send some message from Fragment/ViewModel.
So, to be discussed...