fix: add readOnly prop #4427
Merged
fix: add readOnly prop #4427
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
|
Storybook staging is available at https://kiwicom-orbit-sarka-input-select-mobile-readonly.surge.sh |
|
Size Change: +30 B (+0.01%) Total Size: 445 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
9e6ebc6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://16d49842.orbit.pages.dev |
| Branch Preview URL: | https://sarka-input-select-mobile-re.orbit.pages.dev |
sarkaaa
commented
Jul 25, 2024
sarkaaa
commented
Jul 25, 2024
sarkaaa
commented
Jul 25, 2024
packages/orbit-components/src/InputSelect/InputSelect.ct-story.tsx
Outdated
Show resolved
Hide resolved
|
In general, I have added readOnly prop to In case it's Or would you suggest solving this issue in any other way? 🤔 cc @DSil |
sarkaaa
force-pushed
the
sarka/input-select-mobile-readonly
branch
from
8d08ae5 to
53cdd7c
Compare
DSil
requested changes
Jul 26, 2024
There was a problem hiding this comment.
The described issue is precisely the InputSelect being readOnly on mobile. That is not expected, but I see it still happens here. I also don't understand why forcing it to be readOnly in all stories. The idea is that it is readOnly only when the readOnly prop is actually passed to the component.
sarkaaa
force-pushed
the
sarka/input-select-mobile-readonly
branch
4 times, most recently
from
981e73b to
22a7f61
Compare
I don't understand, the
I set the readOnly prop as a knob.
|
sarkaaa
force-pushed
the
sarka/input-select-mobile-readonly
branch
7 times, most recently
from
24540e0 to
954237b
Compare
DSil
reviewed
Jul 30, 2024
packages/orbit-components/src/InputSelect/__tests__/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/InputSelect/__tests__/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/InputSelect/__tests__/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/InputSelect/InputSelect.ct-story.tsx
Outdated
Show resolved
Hide resolved
sarkaaa
force-pushed
the
sarka/input-select-mobile-readonly
branch
4 times, most recently
from
a4958c2 to
4e33b40
Compare
sarkaaa
commented
Jul 31, 2024
sarkaaa
commented
Jul 31, 2024
| expect(input).toHaveValue(jetLiOption.title); | ||
|
|
||
| await user.tab(); | ||
| expect(onFocus).toHaveBeenCalled(); |
There was a problem hiding this comment.
I call this assertion for every of event (fireEvent, user.tab, user.type)
There was a problem hiding this comment.
I removed tests for events "Enter" and "onKeyDown". I think they can't be triggered from the mobile view (?).
There was a problem hiding this comment.
Asserting the same function call repeatedly is not worth here. It will always return true if it was called at least once. We need to use toHaveBeenCalledTimes and increment the number of times.
Actually, that is good to see that the test was not asserting correctly. Because the second call (after click) was actually not happening.
Because if we click on something that is already focused, the onFocus call won't happen again. We need to blur it first. So we need to have input.blur() after the first assertion.
Then, there is a technical limitation on fireEvent.click, as it does not work for readOnly elements. Therefore, we need to use userEvent event here. Something like await user.click(input). That way I believe we will be able to assert the second onFocus call correctly.
sarkaaa
commented
Jul 31, 2024
|
Hi @DSil |
DSil
reviewed
Jul 31, 2024
| expect(input).toHaveValue(jetLiOption.title); | ||
|
|
||
| await user.tab(); | ||
| expect(onFocus).toHaveBeenCalled(); |
There was a problem hiding this comment.
Asserting the same function call repeatedly is not worth here. It will always return true if it was called at least once. We need to use toHaveBeenCalledTimes and increment the number of times.
Actually, that is good to see that the test was not asserting correctly. Because the second call (after click) was actually not happening.
Because if we click on something that is already focused, the onFocus call won't happen again. We need to blur it first. So we need to have input.blur() after the first assertion.
Then, there is a technical limitation on fireEvent.click, as it does not work for readOnly elements. Therefore, we need to use userEvent event here. Something like await user.click(input). That way I believe we will be able to assert the second onFocus call correctly.
|
|
||
| // Assert that input field still includes the same value | ||
| await user.type(input, "Arnold"); | ||
| expect(onFocus).toHaveBeenCalled(); |
There was a problem hiding this comment.
This is not correct! And it was passing precisely because this assertion was done incorrectly. If the input is readOnly, the user.type event should have no effect. And because it has no effect, the onChange is never called. Therefore, the onFocus is also never called, as expected. So we have two options here:
- We either remove this interaction entirely (why would we test
user.typeon a readonly input?) - We change the assertion to assert the same amount of calls as in the assertion before, to ensure that
onFocuswas not called
There was a problem hiding this comment.
I went with 2nd option, I would like to ensure that typing in readOnly prop is not possible and the value is still the same.
There was a problem hiding this comment.
That is ok, but
I would like to ensure that typing in readOnly prop is not possible
This is not our responsibility in the first place. We should not be testing native behaviors 😄
sarkaaa
force-pushed
the
sarka/input-select-mobile-readonly
branch
from
4e33b40 to
c86ce46
Compare
sarkaaa
force-pushed
the
sarka/input-select-mobile-readonly
branch
from
c86ce46 to
9e6ebc6
Compare
DSil
approved these changes
Aug 1, 2024
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.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.tsCloses https://kiwicom.atlassian.net/browse/FEPLT-2061