ALTAPPS-557: StepQuizHints integrate State to ViewState mapping #330
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
github-actions
bot
added
android
XanderZhu
reviewed
Jan 26, 2023
...rc/commonMain/kotlin/org/hyperskill/app/step_quiz_hints/presentation/StepQuizHintsFeature.kt
Outdated
Show resolved
Hide resolved
XanderZhu
reviewed
Jan 26, 2023
...Main/kotlin/org/hyperskill/app/step_quiz_hints/presentation/StepQuizHintsActionDispatcher.kt
Outdated
Show resolved
Hide resolved
XanderZhu
reviewed
Jan 26, 2023
...rc/commonMain/kotlin/org/hyperskill/app/step_quiz_hints/view/model/StepQuizHintsViewState.kt
Outdated
Show resolved
Hide resolved
XanderZhu
reviewed
Jan 26, 2023
Comment on lines
8
to
+17
| SEEN(JsonPrimitive("seen")), | ||
| HELPFUL(JsonPrimitive("helpful")), | ||
| UNHELPFUL(JsonPrimitive("unhelpful")), | ||
| UNHELPFUL(JsonPrimitive("unhelpful")); | ||
|
|
||
| companion object { | ||
| private val VALUES: Array<HintState> = values() | ||
|
|
||
| fun getByStringValue(value: String): HintState? = | ||
| VALUES.firstOrNull { it.userStorageValue.jsonPrimitive.content == value } | ||
| } |
There was a problem hiding this comment.
enums can be automatically serialized using kotlinx.serialization. You don't need to use JsonProomotive.
In this case you just need to mark each invariant with @SerialName("originName") annotation.
See documentation.
There was a problem hiding this comment.
Also, enumentry it's own built-in name and can be created via.valueOf()` method.
See documentation.
There was a problem hiding this comment.
The main usage of the HintState is in StepQuizHintsActionDispatcher when updating user storage via UserStorageInteractor. The UserStorageInteractor API works with the JSON types directly (see /user_storage/domain/model/UserStorage.kt). So it's currently impossible to implement the proposed changes and it's out of the scope of this PR.
XanderZhu
added
awaiting changes
vladkash
approved these changes
Jan 26, 2023
...rc/commonMain/kotlin/org/hyperskill/app/step_quiz_hints/view/model/StepQuizHintsViewState.kt
Outdated
Show resolved
Hide resolved
ivan-magda
added
awaiting review
XanderZhu
approved these changes
Feb 1, 2023
XanderZhu
added
ready to pull
ivan-magda
deleted the
feature/ALTAPPS_557/integrate_state_to_viewstate_mapping
branch
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-557
Checklist
Before Code Review:
Description
For iOS, there are no differences in what to use
Feature.StateorFeature.ViewState-> we are just working with some state. The primary pain for iOS (like before withFeature.State) is when we are frequently using asealed interface. We need to addEquatableconformance to those states to have the ability to notify View about data changes only when they really occurred (see for exampleStepQuizHintsViewStateKsExtensions.swift). Hope in the future we will move this into shared logic.In general, I like that kind of ViewState. We are not exposing Feature.State to the platform code, but only required ViewState for displaying the UI on the platforms.
Also, see my comments in the code.