-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Get environment and select resource by qualifiers #4018
Conversation
...sources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.kt
Outdated
Show resolved
Hide resolved
...ources/library/src/iosMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.ios.kt
Outdated
Show resolved
Hide resolved
...es/library/src/macosMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.macos.kt
Outdated
Show resolved
Hide resolved
e88fa3d
to
5208ef6
Compare
5208ef6
to
3f0c12e
Compare
components/resources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/Qualifier.kt
Outdated
Show resolved
Hide resolved
...ibrary/src/desktopMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.desktop.kt
Show resolved
Hide resolved
internal actual fun getResourceEnvironment(): ResourceEnvironment { | ||
val locale = Intl.Locale(window.navigator.language) | ||
val isDarkTheme = window.matchMedia("(prefers-color-scheme: dark)").matches | ||
val dpi: Int = (window.devicePixelRatio * 96).toInt() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make 96
a constant, not just magic number. Or extension function for conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add just a comment here. Constant with the only usage is overkill
val displaySizePX = CGDisplayPixelsWide(screenNumber).toFloat() * backingScaleFactor | ||
val displaySizeMM = CGDisplayScreenSize(screenNumber).useContents { width } | ||
|
||
//1 inch = 25.4 mm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same. It's better to have named constant or explicit conversion function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no a reason to have such small granularity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function should be easy readable. Granularity and cleanness are tools for achieve that, not otherwise
...s/demo/shared/src/commonMain/kotlin/org/jetbrains/compose/resources/demo/shared/StringRes.kt
Outdated
Show resolved
Hide resolved
items = setOf(ResourceItem(emptySet(), path)) | ||
) | ||
|
||
/** | ||
* Retrieves a [Painter] using the specified image resource. | ||
* Retrieves a [Painter] using the specified drawable resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add more details about file types (extensions) we support.
...sources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.kt
Show resolved
Hide resolved
...sources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.kt
Outdated
Show resolved
Hide resolved
...sources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.kt
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResClassTask.kt
Show resolved
Hide resolved
gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/ResourcesSpec.kt
Show resolved
Hide resolved
...sources/library/src/commonMain/kotlin/org/jetbrains/compose/resources/ResourceEnvironment.kt
Show resolved
Hide resolved
qualifiersMap[regionQualifier]?.let { q -> | ||
val lang = qualifiersMap[languageQualifier] | ||
if (lang == null) { | ||
error("Region qualifier must be used only with language") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May we provide a context here for users to understand where they did mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add dot to the end of the sentence
No description provided.