Add opportunity to use custom prefixes in StyleSheet #3015
Conversation
|
Hello everybody :) Is there any chance to see this patch in the master? |
|
@eymar hello, could you please review this PR? It would be very useful to see it in master and in next update of compose (for example, |
| ) : StyleSheetBuilder, CSSRulesHolder by rulesHolder { | ||
| private val boundClasses = mutableMapOf<String, CSSRuleDeclarationList>() | ||
| val prefix = prefix ?: "${this::class.simpleName}-" |
There was a problem hiding this comment.
does prefix need to be public? can it be private?
Also to preserve the compatibility, maybe it's worth to add a public val usePrefix with only getter that returns true when prefix is not null.
I gues val usePrefix didn't need to be public at all, but since it's already here, it's better to avoid breaking changes.
There was a problem hiding this comment.
to see it in master and in next update of compose
it will likely go into 1.6
There was a problem hiding this comment.
Looks like I resolved this point
|
@eymar Thank you for your review! ❤️ I have added tests and fixed the points you mentioned above |
| @@ -17,13 +17,13 @@ class StyleSheetTests { | |||
|
|
|||
| @Test | |||
| fun extendExistingStyleSheet() { | |||
| val styleSheet = object : StyleSheet(usePrefix = false) { | |||
There was a problem hiding this comment.
let's keep the old constructor test here too.
Feel free to add a copy of this test with new constructor
| @@ -38,12 +41,22 @@ class CSSRulesHolderState : CSSRulesHolder { | |||
| * ``` | |||
| */ | |||
| open class StyleSheet( | |||
| prefix: String?, | |||
There was a problem hiding this comment.
Does it make sense in your opinion to name this constructor parameter something like customNamePrefix or just customPrefix?
So it's not confused with protected val prefix: String...
Then it will be easier with this line:
val usePrefix: Boolean = prefix == null // by looking at this line it's not clear right away what prefix is meant here (from constrcutor of the property)
There was a problem hiding this comment.
renamed to customPrefix :)
No description provided.