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

[Feature] Asset Provider #13

Closed
wants to merge 10 commits into from
Closed

[Feature] Asset Provider #13

wants to merge 10 commits into from

Conversation

hopeman15
Copy link
Contributor

name: Asset Provider
about: A crazy first approach for accessible assets 😅
label: 'WIP'


Description

This is just an idea to handle assets in a more accessible way. There are a few things that I still don't like, such as passing the android context around. I did limit this as best as possible and would work to eliminate it altogether so that one can access the assets without it. ChaiImage.DCKE_MAIN vs. ChaiImage.DCKE_MAIN(ctx).

⚠️ I didn't want to spend too much more time on this if this isn't a viable solution or something we want to pursue further.

Anyway, let me know what you think 🙌 Let's discuss this one 🤓


How can this PR be tested?

You can test it in a few ways:

  • Run the tests 🧪
  • Run ChaiDemo, here you can see the idea in action 🚀

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Ensure your branch is up to date with develop branch.
  • Ensure your branch runs without breaking.
  • Ensure you solved any merge conflicts.
  • Smaller PRs for the ease of reviewing
  • Make sure to open a GitHub issue as a bug/feature request before writing your code! This means that we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass (./gradlew --init-script gradle/init.gradle.kts spotlessApply to automatically apply formatting)
  • Appropriate docs were updated (You are adding code so...)
  • Tests have been written

Is this your first Pull Request?

  • Run ./tools/setup.sh
  • Import the code formatting style as explained in the setup script.

Fixes #<issue_number_goes_here> 🦕

Screenshot

Screenshot_20230324_204158

@hopeman15 hopeman15 requested a review from tamzi March 24, 2023 19:45
@hopeman15 hopeman15 marked this pull request as ready for review March 24, 2023 19:45
@hopeman15 hopeman15 mentioned this pull request Mar 24, 2023
2 tasks
block()
} catch (e: FileNotFoundException) {
// Throw chai exception instead
throw ChaiException("${e.message} is not located in asset folder")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd extend this message to say which asset folder:

  • Image
  • Icon
  • etc.


object AssetProvider {

fun image(ctx: Context, path: String): Bitmap =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to add support for other asset types, using generics to reduce the amount of redundancy in creating a bunch of new functions

@tamzi
Copy link
Contributor

tamzi commented Mar 26, 2023

@hopeman15 are we merging this or leaving a WIP tag on it for a future reWork?

@hopeman15
Copy link
Contributor Author

hopeman15 commented Mar 26, 2023

@hopeman15 are we merging this or leaving a WIP tag on it for a future reWork?

@tamzi So this isn't done 🙈 I wanted your feedback first to decide whether or not it makes sense to pursue this. Is this what you were thinking, or doe this go in a completely different direction?

@tamzi
Copy link
Contributor

tamzi commented Mar 26, 2023

LGTM 👋🏿

@tamzi
Copy link
Contributor

tamzi commented Mar 26, 2023

or in full..makes sense to pursue it! 😄

@tamzi
Copy link
Contributor

tamzi commented Mar 30, 2023

@hopeman15 An interesting improvement: check discussion on this topic here: #19

@hopeman15 hopeman15 closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants