Skip to content

Conversation

@stefan-niedermann
Copy link
Member

@stefan-niedermann stefan-niedermann commented Feb 10, 2023

Fixes #5

@stefan-niedermann stefan-niedermann linked an issue Feb 11, 2023 that may be closed by this pull request
Copy link
Member

@AlvaroBrey AlvaroBrey left a comment

Choose a reason for hiding this comment

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

I'll get back to this when I have time to contribute, but this needs several improvements:

  • The API is unstable for now, and can/will have breaking changes until 1.0 is released. This should be noted in the README.
  • The setup info should include that Jitpack is needed in the buildscript/dependencyResolution config
  • This repo publishes namespaced (with artifactId) libraries. This is because in the future it will include several different libraries for different usages (see #56). As such, in order to use the theming library, the artifact should be com.github.nextcloud.android-common:ui:version rather than com.github.nextcloud:android-common:version.
  • Though it is possible to instantiate things manually, the theming lib is intended to be used via a DI framework, so that it is not needed to pass things around so much. I might change how the theming modules are created (with a factory or something) to make them easier to use without DI in the future.
  • Additionally example should be in Kotlin.
  • Known users is not needed, IMO

@stefan-niedermann
Copy link
Member Author

Well, it was meant as a starting point, because currently there is no README at all.

I agree to most of your points, but

The API is unstable for now, and can/will have breaking changes until 1.0 is released. This should be noted in the README.

is already clarified by semantic versioning

and

Though it is possible to instantiate things manually, the theming lib is intended to be used via a DI framework, so that it is not needed to pass things around so much. I might change how the theming modules are created (with a factory or something) to make them easier to use without DI in the future.

Yoir intention is clear, however it is up to the user IMHO to decide whether a DI framework is used or not 🤷

@AlvaroBrey
Copy link
Member

Yoir intention is clear, however it is up to the user IMHO to decide whether a DI framework is used or not

Yeah, that's why I said I may change the API to support easier creation without DI.

Either way don't take my comments the wrong way, I really appreciate that you started with the README. I'll polish it a bit when I have the time.

@AndyScherzinger AndyScherzinger added the 3. to review Waiting for reviews label Feb 14, 2023
Copy link
Member

@AndyScherzinger AndyScherzinger left a comment

Choose a reason for hiding this comment

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

better than none, hence merging

@AndyScherzinger AndyScherzinger merged commit 7e94b16 into main Apr 9, 2023
@AndyScherzinger AndyScherzinger deleted the readme branch April 9, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add README

4 participants