Refactor Add support for heightDp, widthDp, showBackground, backgroundColor

[takahirom]

No description provided.

[github-actions]

Snapshot diff report

File name	Image
com.github.takahirom .roborazzi.sample.Co mposeTest.accessibil ityExplanation_2_com pare.png
com.github.takahirom .roborazzi.sample.Co mposeTest.accessibil ityExplanation_merge d_compare.png
com.github.takahirom .roborazzi.sample.Co mposeTest.accessibil ityExplanation_compa re.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewWithPro perties1.Preview_Nam e_Preview_Group_API_ LEVEL_30_W320dp_H640 dp_ja-rJP_FONT_1_5f_ compare.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewShowBac kgroundWithoutBackgr oundColor.Preview_sh owBackground_only_WI TH_BACKGROUND_compar e.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewNormal_ compare.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewWithWid th.Preview_width_API _LEVEL_30_W500dp_com pare.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewWithPro perties2.WITH_SYSTEM UI_WITH_BACKGROUND BG_COLOR_4278190335_ NIGHT_NEXUS_5_WALLPA PER_GREEN_DOMINATED_ compare.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewDarkMod e.NIGHT_compare.png
com.github.takahirom .preview.tests.Previ ewsKt.PreviewWithHei ght.Preview_height_H 500dp_compare.png

[sergio-sastre]

@takahirom
I just left a couple of general remarks.

Feel free to address them if you find them appropriate

[sergio-sastre]

it is true that it adds some complexity, but brings the benefit of making the api more flexible.

Therefore, I have a couple of points:

1. We are inconsistent in the way we handle fontSize, UiMode, Locale, device...etc. vs. sized and colored.
   I think it would become much more intuitive if all of them would be handled the same way, e.g.

RoborazziComposeApplierBuilder()
   .sized(
       widthDp = previewInfo.widthDp,
       heightDp = previewInfo.heightDp
     )
     // I'd rather call this background than coloured. Both properties include background inn their names indeed. One could also call them : `show` instead of `showBackground` and `color` instead of `backgroundColor`
     .background( 
       showBackground = previewInfo.showBackground,
       backgroundColor = previewInfo.backgroundColor
     )
     .uiMode(
        configurationUiMode  = previewInfo.uiMode
     )
     .locale(
        bcp47LanguageTag = previewInfo.locale
     )
     ...

I think symmetry in code also makes it more understandable

2. The name of this variable is a bit ambiguous. I'm not sure what to expect from this as a user of this method. I think something in the direction of previewOptions and RoborazziPreviewOptionsBuilder or composableConfigand RoborazziComposableConfigBuilder is more understandable

[takahirom]

Thank you for your review. That makes sense.
I've migrated the settings such as local to the applier and renamed the applier to config.
a899893
https://github.com/takahirom/roborazzi/pull/579/files

[sergio-sastre]

Looks good to me.
The only improvements I’d propose are input validations (e.g. fontScale should not be lower than 0 and the like) and logging messages for invalid inputs.

apart from that feel free to merge 😊