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

Explicit opt in to RTL support #575

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

jrodbx
Copy link
Collaborator

@jrodbx jrodbx commented Sep 14, 2022

In production apps, RTL support is enabled from the application manifest.
In LayoutLib, RTL support is enabled in RenderParams.setRtlSupport, which was set to true as part of this PR.

However, while testing internally on CashApp, it looks like a few screens (primarily due to this Contour issue) break with this change, so let's enable explicit opt-in for those using Contour, until the upstream issue is fixed.

Note the horizontal margins on the X and Search icons in the toolbar in the following screenshots from a production app, when the aforementioned manifest attribute is enabled:

Screenshot_20220913-233354Screenshot_20220913-233547

@jrodbx jrodbx force-pushed the jrod/2022-09-13/explicitly-support-rtl branch from 7dbf17b to 927c289 Compare September 14, 2022 03:56
@@ -86,7 +86,8 @@ class Paparazzi @JvmOverloads constructor(
private val appCompatEnabled: Boolean = true,
private val maxPercentDifference: Double = 0.1,
private val snapshotHandler: SnapshotHandler = determineHandler(maxPercentDifference),
private val renderExtensions: Set<RenderExtension> = setOf()
private val renderExtensions: Set<RenderExtension> = setOf(),
private val supportsRtl: Boolean = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val supportsRtl: Boolean = false
private val supportsRtl: Boolean = false,

@@ -158,7 +159,8 @@ class Paparazzi @JvmOverloads constructor(
.copy(
layoutPullParser = LayoutPullParser.createFromString(contentRoot),
deviceConfig = deviceConfig,
renderingMode = renderingMode
renderingMode = renderingMode,
supportsRtl = supportsRtl
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
supportsRtl = supportsRtl
supportsRtl = supportsRtl,

@@ -48,7 +48,8 @@ internal data class SessionParamsBuilder(
private val layoutPullParser: LayoutPullParser? = null,
private val projectKey: Any? = null,
private val minSdk: Int = 0,
private val decor: Boolean = true
private val decor: Boolean = true,
private val supportsRtl: Boolean = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val supportsRtl: Boolean = false
private val supportsRtl: Boolean = false,

@JakeWharton
Copy link
Contributor

If you want you can force trailing commas with ktlint to keep diffs like this clean with only added lines instead of causing each new parameter to also change a line.

@jrodbx
Copy link
Collaborator Author

jrodbx commented Sep 14, 2022

If you want you can force trailing commas with ktlint to keep diffs like this clean with only added lines instead of causing each new parameter to also change a line.

i tried but spotless complained :( cc: @fcduarte who had looked into this

@jrodbx jrodbx merged commit 9c3a4ff into master Sep 14, 2022
@jrodbx jrodbx deleted the jrod/2022-09-13/explicitly-support-rtl branch September 14, 2022 14:34
@JakeWharton
Copy link
Contributor

It's not the default. You have to opt in so ktlint knows you want it.

@fcduarte
Copy link
Contributor

It's not the default. You have to opt in so ktlint knows you want it.

Yeah, this needs to be added:

        'ij_kotlin_allow_trailing_comma_on_call_site': 'true',
        'ij_kotlin_allow_trailing_comma': 'true'

to spotless. @jrodbx I think we never got to a conclusion if we want the trailing commas to be forced or not. I can put a PR to force if needed, thanks!

@JakeWharton
Copy link
Contributor

JakeWharton commented Sep 14, 2022

If you switch to Kotlinter then you can put those in your editorconfig and both the IDE and ktlint will honor them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants