[RNMobile] Spacer block: Fix import of MIN_SPACER_SIZE constant
#40053
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.22 MB ℹ️ View Unchanged
|
fluiddot
added
[Type] Bug
hypest
reviewed
Apr 6, 2022
|
|
||
| // Open block settings | ||
| fireEvent.press( getByA11yLabel( 'Open Settings' ) ); | ||
| await waitFor( |
There was a problem hiding this comment.
Not a blocker and something to consider for a follow-up PR, could we introduce a helper that would make this wait be used like:
await waitForTestId( 'block-settings-modal' );There was a problem hiding this comment.
Sure, it would be great if we had more helpers to reduce code in tests and make it easier to read. Actually, some time ago I created this PR #38619 where I added several helpers, however, I haven't spared enough time to push it. For this case, we could use the helper openBlockSettings which will wait for the block settings modal to be opened.
Regarding the helper you mentioned (i.e. waitForTestId), I'm not sure if it would fit the requirement here as we're not only waiting for a component with a test Id but also for the modal to be visible. The way I see the implementation of that helper would be:
const waitForTestId = ( testId ) => {
return waitFor( () => getByTestId( testId) );
};
However, in this case, we're also looking for the prop isVisible to be true:
getByTestId( 'block-settings-modal' ).props.isVisible
There was a problem hiding this comment.
Sorry for not making it clear before, I think the helper should include the truth-check for isVisible. Do you perhaps mean that sometimes we want to check it and others not?
There was a problem hiding this comment.
Sorry for not making it clear before, I think the helper should include the truth-check for isVisible.
Oh ok, that makes sense, although not every component has the isVisible prop. I think that in this case, the helper would be specific to modals or another type of component that its visibility is determined by a prop (although can't think of another component 🤔 ). Hence, we could name it differently like waitForModal or waitForModalByTestId instead, wdyt?
Do you perhaps mean that sometimes we want to check it and others not?
Yep, in some cases, usually where state updates occur, we need to wait for a component to be rendered via the waitFor. That's why I thought that the helper waitForTestId would behave as I outlined in this comment.
There was a problem hiding this comment.
Hence, we could name it differently like waitForModal or waitForModalByTestId instead, wdyt?
Sounds good to me. In the end, my main goal with the comment thread here is to see if we can reduce the verbosity of the code, so anything towards that end I'm 👍
Yep, I think we could remove the ones that don't require checking visual changes. Besides, I noticed that they're a bit outdated as we no longer have a slider to modify the |
SiobhyB
pushed a commit
that referenced
this pull request
Apr 7, 2022
…0053) * Fix import of MIN_SPACER_SIZE constant * Add tests for Spacer block
4 tasks
hypest
reviewed
Apr 15, 2022
| @@ -14,7 +14,7 @@ import { __ } from '@wordpress/i18n'; | |||
| /** | |||
| * Internal dependencies | |||
| */ | |||
| import { MIN_SPACER_SIZE } from './edit'; | |||
| import { MIN_SPACER_SIZE } from './edit.js'; | |||
There was a problem hiding this comment.
Hmm, can we rethink the implementation here? I think it's highly unintuitive to use the extension-specific form here to load the constant from the web variant of the file and I think can easily lead to hard to find bugs.
Instead, can we define the constant in a constants.js file? that file won't be forked unless it needs to and define the value for both the web and native mobile to use. WDYT @fluiddot ?
There was a problem hiding this comment.
Hmm, can we rethink the implementation here? I think it's highly unintuitive to use the extension-specific form here to load the constant from the web variant of the file and I think can easily lead to hard to find bugs.
Instead, can we define the constant in a
constants.jsfile? that file won't be forked unless it needs to and define the value for both the web and native mobile to use. WDYT @fluiddot ?
Good point and I totally agree, I'll open a new PR with that change 👍 .
There was a problem hiding this comment.
FYI in this PR we'll add the constants file.
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fix import of
MIN_SPACER_SIZEconstant in Spacer block.Why?
After the changes introduced in #39577, the
MIN_SPACER_SIZEconstant is now used as theminprop passed to theUnitControlcomponent (reference). However, the current import statement (reference) is pointing to the native version of theeditfile instead of the web version, where the constant is defined (reference).This implies that the value used in the
minprop isundefined, which leads to a crash in the app when theUnitControlcomponent is focused.How?
Use
./edit.jsas the import path instead of./editto assure we load the constant from the righteditfile.Testing Instructions
Screenshots or screencast
N/A