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

Registration #5: Add e-mail entry layout #11

Merged
merged 7 commits into from
Jun 17, 2020
Merged

Conversation

gizemb
Copy link

@gizemb gizemb commented Jun 16, 2020

  • Sets up dependency injection infrastructure
  • Adds TextInputLayout style
  • Adds circular button

TODO: Still needs updating cursor color on editable fields

Screenshot_1592298272

Comment on lines +27 to +29
kotlinOptions {
jvmTarget = JavaVersion.VERSION_1_8.toString()
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@android10 is there a better place to put this? I couldn't put it under compilation.gradle.kts because it requires kotlin plugin.

Copy link

@BradleyIW BradleyIW Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we not add the kotlin gradle plugin to the buildSrc/build.gradle.kts dependencies closure to allow for this to be used within the scripts? (also could you make the Dependencies object private in there too if we do decide to add this plugin)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need some guidance from Fernando on this. I couldn't even create a .gradle.kts file :D

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gizem I would not start mixing things up. AFAIK we do not need this right? By default it compiles against Java 8.
In case you need it, maybe I can raise a PR tomorrow and provide more context but I would extract all this and keep the build.gradle.kts as small and clean possible.

@BradleyWire I do not understand your point. "Dependencies" cannot be private. buildSrc is a separated project that builds stuff up, which becomes available to the rest of gradle scripts.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@android10 Dependencies is the object inside your build.gradle.kts inside the buildSrc module, it doesn't need to have public access seeing as it's only constant belongs in the same file as it's own

object Dependencies {
    const val AndroidBuildTools = "com.android.tools.build:gradle:4.0.0"
}

I'd be safe in assuming this only need to be private to the file. It's a minor cosmetic change and is out of the scope of this PR which is why I asked to only change it if you're adding a dependency change to the gradle file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I thought you were talking about something else.
Here is the fix:
https://github.com/wireapp/wire-android-reloaded/pull/12/files

Copy link

@BradleyIW BradleyIW Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @android10, Also, you'll need the kotlinOptions closure eventually otherwise you'll run into a complaint from the Kotlin compiler:

Cannot inline bytecode built with JVM target 1.8 into bytecode that is being built with JVM target 1.6

I've ran into this previously with the observe(..) extension function when observing a LiveData instance (which is heavily used in our architecture)

@gizemb gizemb force-pushed the feature/registration_part_5 branch from 864c1ae to ed3a88c Compare June 16, 2020 13:50
Copy link

@android10 android10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a couple of points that need and answer. Then 👍 😄

Comment on lines +27 to +29
kotlinOptions {
jvmTarget = JavaVersion.VERSION_1_8.toString()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gizem I would not start mixing things up. AFAIK we do not need this right? By default it compiles against Java 8.
In case you need it, maybe I can raise a PR tomorrow and provide more context but I would extract all this and keep the build.gradle.kts as small and clean possible.

@BradleyWire I do not understand your point. "Dependencies" cannot be private. buildSrc is a separated project that builds stuff up, which becomes available to the rest of gradle scripts.

private val featureModules: List<Module> = listOf(registrationModule)

fun start(context: Context) {
startKoin {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I pointed out in the other PR with the styles: Brackets {} are fine to keep them this way in order to favor the readability of the expression, but the arguments...mmm...I do not see it and I think we are not writing concise Kotlin code. This is how it would look for me:

        startKoin {
            androidContext(context)
            modules(listOf(coreModules, sharedModules, featureModules).flatten())
        }

I would also be of the idea of extracting the list of modules to a constant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combined them into a single line. I didn't feel the need to create a new variable for the module list. If we have one more in the future, I can add it.

Base automatically changed from feature/registration_part_4 to master June 17, 2020 12:27
Copy link

@android10 android10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only added a tiny thing apart from my last comments. Then 👍

Comment on lines +27 to +29
kotlinOptions {
jvmTarget = JavaVersion.VERSION_1_8.toString()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I thought you were talking about something else.
Here is the fix:
https://github.com/wireapp/wire-android-reloaded/pull/12/files

Copy link

@android10 android10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was such a great discussion. Great Job @gizemb 👍

@wireapp wireapp deleted a comment Jun 17, 2020
@gizemb gizemb merged commit 99cb910 into master Jun 17, 2020
@gizemb gizemb deleted the feature/registration_part_5 branch June 17, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants