Issue/12597 custom fields banner #12601
Conversation
| private val appPrefs: AppPrefsWrapper = mock { | ||
| val bannerDismissed = MutableStateFlow(false) | ||
| on { observePrefs() }.thenReturn(bannerDismissed.map { Unit }) | ||
| on { isCustomFieldsTopBannerDismissed } doAnswer { bannerDismissed.value } | ||
| on { isCustomFieldsTopBannerDismissed = any() }.then { invocation -> | ||
| bannerDismissed.update { invocation.arguments[0] as Boolean } | ||
| } | ||
| } |
There was a problem hiding this comment.
This works more like a fake than a mock, and allows for a more thorough testing (we can simulate the dismiss properly).
hichamboushaba
added
type: enhancement
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
hichamboushaba
force-pushed
the
issue/12597-custom-fields-banner
branch
from
89982ce
to
bb72363
Compare
hichamboushaba
requested review from
JorgeMucientes and
irfano
and removed request for
JorgeMucientes
There was a problem hiding this comment.
Nice job @hichamboushaba, the code looks good and works as expected. I left a minor suggestion about displaying the banner message in expanded state by default for this particular case. Lmk wdyt.
| shape = RectangleShape, | ||
| modifier = modifier | ||
| ) { | ||
| var isExpanded by rememberSaveable { mutableStateOf(false) } |
There was a problem hiding this comment.
Minor suggestion.
For this particular case where the text is short, very relevant and there's plenty of space on the screen, wdyt of displaying the text expanded by default? We can add a new parma to the composable, something like initiallyExapanded : Boolean = false and use it to set the initial status of it.
There was a problem hiding this comment.
Thanks Jorge, great idea! I implemented this, but the banner will collapse by default in landscape mode due to the reduced screen real estate.
Closes: #12597
Description
This PR adds a banner to the custom fields list screen to show a message that would help explaining how the saving works for them, in contrast to how it works in the other parts of the Product Details.
For more information on why we need this and how we reached this decision please check this discussion p1725968896022239/1725948493.332539-slack-C03L1NF1EA3
Steps to reproduce
Testing information
The tests that have been performed
The above.
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: