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

Migrate HelpFragment to jetpack. #4159

Open
MohitMaliFtechiz opened this issue Jan 4, 2025 · 10 comments · May be fixed by #4174
Open

Migrate HelpFragment to jetpack. #4159

MohitMaliFtechiz opened this issue Jan 4, 2025 · 10 comments · May be fixed by #4174
Assignees
Milestone

Comments

@MohitMaliFtechiz
Copy link
Collaborator

Parent Issue #2669

@kelson42 kelson42 added this to the 3.15.0 milestone Jan 4, 2025
@jackq97
Copy link

jackq97 commented Jan 8, 2025

Hello, i would like to take this issue and migrate the help fragment to jetpack compose screen . you can assign this issue to me.

@MohitMaliFtechiz
Copy link
Collaborator Author

@jackq97 Thanks for showing the interest. @SOUMEN-PAL requested to start working on this issue, @SOUMEN-PAL can you please confirm are you working on this issue?

@SOUMEN-PAL
Copy link

@jackq97 Thanks for showing the interest. @SOUMEN-PAL requested to start working on this issue, @SOUMEN-PAL can you please confirm are you working on this issue?

Yes I am working on the problem.
I have sorted out the changes with the ui and regarding the viewmodel for proper migration.
If needed I can also update the XML to be compatible with compossable retaining the XML addons.

@MohitMaliFtechiz
Copy link
Collaborator Author

@SOUMEN-PAL Thanks for confirming, I have assigned the issue to you. Please make a PR when you are done.

If needed I can also update the XML to be compatible with compossable retaining the XML addons.

Is it required somewhere? Do if it is necessary for now, we will remove them once we migrate all the things in jetpack.

@SOUMEN-PAL
Copy link

SOUMEN-PAL commented Jan 12, 2025

Hello @MohitMaliFtechiz
The migration of HelpFragment is about to be completed and here is an update about how it going on.
The code is entirely written in jetpack Compose as required.
(A little bit of enhancement are left and will soon be sending a PR in upcoming days after completing.)

Task.mp4

I just have a question about the linter which is needed during commit, do I need to manually adhere to all the things or is there any automatic solution.

@MohitMaliFtechiz
Copy link
Collaborator Author

@SOUMEN-PAL Thanks for the update.

I just have a question about the linter which is needed during commit, do I need to manually adhere to all the things or is there any automatic solution.

You have to fix those lint issues. There are some automatic solutions, like formatting and removing unused imports. But apart from this, you have to manually fix other lint problems.

@SOUMEN-PAL
Copy link

SOUMEN-PAL commented Jan 14, 2025

@MohitMaliFtechiz The Migration is done completely here is the video below. I am having problem with rules in Lint as compossable functions has a naming convention starting with capital Letters and the rules in lint says it has to be small.
When i force the compossable names to be in ( camelCase ) the initial rules are successes but due to improper Name of compossable lint gives a new warning.
I am not seeing any way to send a Commit and request a pull request.

(commit with --no-verify is possible but i need permission should I do that?)

MigrationHelp.mp4

and here is the example of the kind of Lint Errors I am getting:-

FunctionNaming - [Function names should match the pattern: ^([a-z$][a-zA-Z$0-9])|(.*)$] at /home/soumen/Documents/kiwix-android/core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpItem.kt:70:5
FunctionNaming - [Function names should match the pattern: ^([a-z$][a-zA-Z$0-9]
)|(.*)$] at /home/soumen/Documents/kiwix-android/core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpItem.kt:124:5
FunctionNaming - [Function names should match the pattern: ^([a-z$][a-zA-Z$0-9])|(.*)$] at /home/soumen/Documents/kiwix-android/core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpScreen.kt:68:5
FunctionNaming - [Function names should match the pattern: ^([a-z$][a-zA-Z$0-9]
)|(.*)$] at /home/soumen/Documents/kiwix-android/core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpScreen.kt:95:13
FunctionNaming - [Function names should match the pattern: ^([a-z$][a-zA-Z$0-9])|(.*)$] at /home/soumen/Documents/kiwix-android/core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpScreen.kt:120:13
FunctionNaming - [Function names should match the pattern: ^([a-z$][a-zA-Z$0-9]
)|(.*)$] at /home/soumen/Documents/kiwix-android/core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpScreen.kt:145:13

SOUMEN-PAL added a commit to SOUMEN-PAL/kiwix-android that referenced this issue Jan 15, 2025
SOUMEN-PAL added a commit to SOUMEN-PAL/kiwix-android that referenced this issue Jan 15, 2025
…wixHelpFragment in app module to run Compossables, with addition of Jetpack compose compatibility in gradel app/core
@SOUMEN-PAL SOUMEN-PAL linked a pull request Jan 15, 2025 that will close this issue
@MohitMaliFtechiz
Copy link
Collaborator Author

@SOUMEN-PAL These are the detekt issue since the project is now starting supporting the compose so the detekt rules were not configured according to compose. Here is the doc of detekt https://detekt.dev/docs/introduction/compose/ how we can configure rules of detekt for compose.

@jackq97
Copy link

jackq97 commented Jan 17, 2025

@SOUMEN-PAL These are the detekt issue since the project is now starting supporting the compose so the detekt rules were not configured according to compose. Here is the doc of detekt https://detekt.dev/docs/introduction/compose/ how we can configure rules of detekt for compose.

@MohitMaliFtechiz We will need to update the detekt configuration file to support composable functions , by changing rules for functions to support naming conventions for composable functions and ignoring annotation for Composable. There are other parameters as well. i can do that to add support for jetpack compose.

@jackq97
Copy link

jackq97 commented Jan 18, 2025

@MohitMaliFtechiz I've made the changes and made a pr you can check it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants