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

Convert gallery from mvp to mvvm #443

Merged
merged 75 commits into from
Oct 20, 2024
Merged

Conversation

hieuwu
Copy link
Collaborator

@hieuwu hieuwu commented Apr 6, 2024

🚀 Summary

Convert gallery from mvp to mvvm
Details of the analysis
https://glamorous-lung-568.notion.site/MVP-to-MVVM-Gallery-screen-analysis-fdb7ac52dfaf4f3a92d4fe0acbd1ab66?pvs=4

🖼️ Screenshots:

Provide screenshots to make it visible to reviewer if possible (Optional)

| Before | After |
N/A

@kirillt
Copy link
Member

kirillt commented Apr 8, 2024

@hieuwu thanks, could you also tell a bit about motivation behind these changes ⏤ what goal you want to achieve by the refactoring? Maybe pros & cons of MVP and MVVM

Am I right, that some parts of current codebase will be removed when you complete this PR? I see only additions right now

@hieuwu
Copy link
Collaborator Author

hieuwu commented Apr 8, 2024

@hieuwu thanks, could you also tell a bit about motivation behind these changes ⏤ what goal you want to achieve by the refactoring? Maybe pros & cons of MVP and MVVM

Am I right, that some parts of current codebase will be removed when you complete this PR? I see only additions right now

Yeah I am writing a document that includes current code execution, pros/cons & how to analyze current code then convert them to MVVM step by step. One thing I can reveal for now is that it would help us a lot in adoption to Jetpack Compose if we want in the future.

Yes, current code could be removed after we complete testing for new one.

Copy link

sonarcloud bot commented Oct 20, 2024

@mdrlzy mdrlzy merged commit 773409b into main Oct 20, 2024
7 checks passed
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.

4 participants