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

Add data binding support for "complete_icon" and "slider_icon" attributes #170

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nataliialapshyna-dna
Copy link
Contributor

@nataliialapshyna-dna nataliialapshyna-dna commented Feb 10, 2022

Description

complete_icon and slider_icon attributes are @DrawableRes Int variables. To be set up via Data binding they should also support the Drawable type.

Related PRs/Issues

#156

Todos

  • Tests - no tests
  • Documentation - example added
  • Screenshots - no screenshots

Copy link
Owner

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thank you for sending this 🙏 I've rebased it for you, as I wanted to play around it with it.

We would need to address the Any field comment, but we can merge it just after

@@ -156,17 +156,34 @@ class SlideToActView @JvmOverloads constructor(
}

/** Custom Slider Icon */
@DrawableRes
var sliderIcon: Int = R.drawable.slidetoact_ic_arrow
var sliderIcon: Any = R.drawable.slidetoact_ic_arrow
Copy link
Owner

@cortinico cortinico Jun 3, 2022

Choose a reason for hiding this comment

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

This is sadly suboptimal :/

We're now exposing a public property which is a Any.
I think we should instead create other two private fields called:

private var sliderIconDrawable
private var completeIconDrawable

And let the BindingAdapter operate on those.

@cortinico cortinico added this to the 0.10.0 milestone Jun 3, 2022
@cortinico cortinico modified the milestones: 0.10.0, 0.11.0 Feb 16, 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