Skip to content
This repository was archived by the owner on Feb 4, 2025. It is now read-only.

Conversation

@antonis
Copy link

@antonis antonis commented Jul 19, 2021

Related PRs

WordPress-Android: wordpress-mobile/WordPress-Android#15129
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3793
gutenberg: WordPress/gutenberg#33544

Description

This PR adds the Gallery v2 Flag to the EditorTheme to be passed down to the Editor

To test

See WordPress/gutenberg#33544

get
@JvmName("setIsFSETheme")
set
@Column var galleryWithImageBlocks: 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.

This works well and I don't have a problem with this approach, I would like your thoughts on the alternative approach I took for iOS.

I didn't want to add a temporary key to the data model so I stored it in what would be an EditorThemeElement as a new type experimentalFeatures to make space for future experiments as well. My overall goal was I don't want to create multiple DB changes if we do a lot of experiments but the risk is my approach is added complexity if we don't do more experimental features.

FSE is the space where I can see more experiments happening. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

This is indeed a good idea @chipsnyder towards future proofing our implementation.
On Android thought it seems that it is a bit tricky to alter the type check in the EditorThemeElement table in order to add the new experimentalFeature type. An alternative would be to drop and recreate the table or create an entirely new table for the experiments. In any case I think this will add more complexity at this point. I would suggest to keep this as is for now and refactor when the need comes up with a future experiment. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, I see the dilemma there. I agree that can be a better item to rework if need at the next instance of an experimental flag.

…-blocks-editor-settings_v2

# Conflicts:
#	fluxc/src/main/java/org/wordpress/android/fluxc/persistence/WellSqlConfig.kt
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 2, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

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

I wasn't able to get the Gallery v2 to appear on this build also while testing the analytics the gallery key always showed up as false.

I think the gap is that in EditorThemeSqlUtils.kt makeEditorTheme doesn't handle saving the boolean but perhaps there is something else I'm missing.

I tested by generating a self-hosted site and confirmed I get the expect value from the API.

…-blocks-editor-settings_v2

# Conflicts:
#	fluxc/src/main/java/org/wordpress/android/fluxc/persistence/WellSqlConfig.kt
@antonis antonis marked this pull request as ready for review September 1, 2021 11:12
@antonis antonis requested a review from mkevins September 1, 2021 11:12
Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Tested via: wordpress-mobile/WordPress-Android#15134 (review)

Thanks Antonis! LGTM!

@antonis antonis merged commit a471f62 into develop Sep 2, 2021
@antonis antonis deleted the rnmobile/refactor/gallery-as-nested-image-blocks-editor-settings_v2 branch September 2, 2021 07:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants