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 dsl for Preference #210

Merged
merged 2 commits into from Sep 3, 2020
Merged

Add dsl for Preference #210

merged 2 commits into from Sep 3, 2020

Conversation

janbina
Copy link
Contributor

@janbina janbina commented Aug 30, 2020

Hi, I recently started using the preference screen dsl and it's awesome, thanks for that.
I encountered one problem though - I needed simple Preference with custom layout and there was no function for that. It makes kinda sense since it is not bound to kotpref property, but it will still be nice to have it, I think. I tried to create the Preference on my own then, but since the preferenceScreen is only set after calling the builder function, there is no way to use it to add the Preference to the screen from within that function, or at least I can't see it.

So I created a function for building the Preference and moved the assignment so one can access the preferenceScreen in the builder function to do any custom stuff.

@chibatching
Copy link
Owner

Hi! Thank you for your pull request!

It looks like a nice idea to support Preference with the custom layout.
But I'm not sure why we have to set preferenceScreen before builder function (because I haven't used a custom layout).
Does setting custom layout require preferenceScreen ?

@janbina
Copy link
Contributor Author

janbina commented Sep 2, 2020

No, this not related to custom layout. Imagine that the user has some CustomPreference : Preference() and wants to use it in between standard preferences, like this:

kotprefScreen(preferences) {
	checkBox(...) {}
	
	preferenceScreen.addPreference(
		CustomPreference()
	)

	checkBox(...) {}  
}

In this scenario, he needs access to the preferenceScreen from the builder function.

@chibatching
Copy link
Owner

Oh, I see! I understand!

I'll probably implement a DSL for adding Custom Preferences later, but I haven't come up with an interface yet, so let's go with this method for now!

Could you rebase this branch with master branch to fix CI error?

@codecov
Copy link

codecov bot commented Sep 3, 2020

Codecov Report

Merging #210 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #210   +/-   ##
=========================================
  Coverage     69.75%   69.75%           
  Complexity      129      129           
=========================================
  Files            25       25           
  Lines           400      400           
  Branches         46       46           
=========================================
  Hits            279      279           
  Misses          101      101           
  Partials         20       20           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e8a79f...bcb6bc0. Read the comment docs.

@janbina
Copy link
Contributor Author

janbina commented Sep 3, 2020

Thanks! Rebase done, tests are green :)

@chibatching chibatching merged commit 93f890c into chibatching:master Sep 3, 2020
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.

None yet

2 participants