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.

Sign up for GitHub

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 opportunity to use custom prefixes in StyleSheet #3015

Merged
merged 9 commits into from
Jan 31, 2024
Merged

Add opportunity to use custom prefixes in StyleSheet #3015

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1a892d4
Add opportunity to use custom prefixes in StyleSheet
InsanusMokrassar Apr 8, 2023
b32a136
compilation fixes
InsanusMokrassar Apr 16, 2023
505ef28
Merge branch 'JetBrains:master' into patch-3
InsanusMokrassar Jan 30, 2024
0aac3d0
make StyleSheet#prefix protected and get back public StyleSheet#usePr…
InsanusMokrassar Jan 30, 2024
bfb7cb1
improve tests for StyleSheet
InsanusMokrassar Jan 30, 2024
727e47b
improve tests for StyleSheet animations
InsanusMokrassar Jan 30, 2024
827d856
rename StyleSheet prefix parameter to customPrefix
InsanusMokrassar Jan 30, 2024
45a4334
use old constructors in StyleSheetTests old test
InsanusMokrassar Jan 30, 2024
cff79b5
update kdocs for StyleSheet
InsanusMokrassar Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions html/core/src/jsMain/kotlin/org/jetbrains/compose/web/css/StyleSheet.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ class CSSRulesHolderState : CSSRulesHolder {
/**
* Represents a collection of the css style rules.
* StyleSheet needs to be mounted.
*
* @param prefix Will be used as prefix with current style. Pass `null` to use default value (classname of realization)
*
* @see [Style]
*
* Example:
Expand All @@ -38,12 +41,21 @@ class CSSRulesHolderState : CSSRulesHolder {
* ```
*/
open class StyleSheet(
prefix: String?,
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to customPrefix :)

private val rulesHolder: CSSRulesHolder = CSSRulesHolderState(),
val usePrefix: Boolean = true,
) : StyleSheetBuilder, CSSRulesHolder by rulesHolder {
private val boundClasses = mutableMapOf<String, CSSRuleDeclarationList>()
val prefix = prefix ?: "${this::class.simpleName}-"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eymar eymar Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

to see it in master and in next update of compose

it will likely go into 1.6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I resolved this point


constructor(
rulesHolder: CSSRulesHolder = CSSRulesHolderState(),
usePrefix: Boolean = true
) : this(
if (usePrefix) null else "",
rulesHolder
)

protected fun style(cssRule: CSSBuilder.() -> Unit) = CSSHolder(usePrefix, cssRule)
protected fun style(cssRule: CSSBuilder.() -> Unit) = CSSHolder(prefix, cssRule)

/**
* Example:
Expand All @@ -69,7 +81,7 @@ open class StyleSheet(
* }
* ```
*/
protected fun keyframes(cssKeyframes: CSSKeyframesBuilder.() -> Unit) = CSSKeyframesHolder(usePrefix, cssKeyframes)
protected fun keyframes(cssKeyframes: CSSKeyframesBuilder.() -> Unit) = CSSKeyframesHolder(prefix, cssKeyframes)

companion object {
private var counter = 0
Expand All @@ -88,13 +100,12 @@ open class StyleSheet(
}
}

protected class CSSHolder(private val usePrefix: Boolean, private val cssBuilder: CSSBuilder.() -> Unit) {
protected class CSSHolder(private val prefix: String, private val cssBuilder: CSSBuilder.() -> Unit) {
operator fun provideDelegate(
sheet: StyleSheet,
property: KProperty<*>
): ReadOnlyProperty<Any?, String> {
val sheetName = if (usePrefix) "${sheet::class.simpleName}-" else ""
val className = "$sheetName${property.name}"
val className = "$prefix${property.name}"
val selector = object : CSSSelector() {
override fun asString() = ".${className}"
}
Expand All @@ -110,15 +121,14 @@ open class StyleSheet(
* See [keyframes]
*/
protected class CSSKeyframesHolder(
private val usePrefix: Boolean,
private val prefix: String,
private val keyframesBuilder: CSSKeyframesBuilder.() -> Unit
) {
operator fun provideDelegate(
sheet: StyleSheet,
property: KProperty<*>
): ReadOnlyProperty<Any?, CSSNamedKeyframes> {
val sheetName = if (usePrefix) "${sheet::class.simpleName}-" else ""
val keyframesName = "$sheetName${property.name}"
val keyframesName = "$prefix${property.name}"
val rule = buildKeyframes(keyframesName, keyframesBuilder)
sheet.add(rule)

Expand Down