ScrollLock: Covert component to TypeScript #42303
Merged
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
walbo
added
[Type] Enhancement
|
Size Change: +1 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Jul 11, 2022
| component: ScrollLock, | ||
| title: 'Components/ScrollLock', | ||
| parameters: { | ||
| controls: { hideNoControlsWarning: true }, |
Comment on lines
+67
to
+79
| <StripedBackground> | ||
| <div>Start scrolling down...</div> | ||
| <ToggleContainer> | ||
| <Button variant="primary" onClick={ toggleLock }> | ||
| Toggle Scroll Lock | ||
| </Button> | ||
| { isScrollLocked && <ScrollLock /> } | ||
| <p> | ||
| Scroll locked:{ ' ' } | ||
| <strong>{ isScrollLocked ? 'Yes' : 'No' }</strong> | ||
| </p> | ||
| </ToggleContainer> | ||
| </StripedBackground> |
There was a problem hiding this comment.
I think the point of this component would be better demonstrated (especially in Docs view) if we wrapped it in some divs that represent the "page body" and the "modal". I'll propose that in a follow-up PR once this is merged.
ciampo
reviewed
Jul 12, 2022
Comment on lines
+14
to
+25
| it( 'locks when mounted', () => { | ||
| expect( document.documentElement ).not.toHaveClass( lockingClassName ); | ||
| render( <ScrollLock /> ); | ||
| expect( document.documentElement ).toHaveClass( lockingClassName ); | ||
| } ); | ||
|
|
||
| it( 'unlocks when unmounted', () => { | ||
| const { unmount } = render( <ScrollLock /> ); | ||
| expect( document.documentElement ).toHaveClass( lockingClassName ); | ||
| unmount(); | ||
| expect( document.documentElement ).not.toHaveClass( lockingClassName ); | ||
| } ); |
There was a problem hiding this comment.
I wonder if using toMatchStyleDIffSnapshot here would make for a better test — basically checking that the difference between locked and unlocked state on document.body only produces an overflow: hidden style diff.
Otherwise, we could completely change the styles associated with the lockscroll CSS class and tests wouldn't break.
There was a problem hiding this comment.
Not sure how to use toMatchStyleDIffSnapshot on the body here 🤔 I tried, but failed to get the style. Are you able to take a look at it @ciampo? Either in this PR or as a follow-up.
There was a problem hiding this comment.
Looking at the implementation of toMatchStyleDiffSnapshot, it may not be able to get inline styles.
Let's get this PR merged nonetheless - thank you for looking into it though!
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?
Converts the
ScrollLockcomponent to TypeScript.Why?
Part of the @wordpress/components's TypeScript migration (#35744).
How?
ScrollLockto TypeScript.@testing-library/reactand TypeScriptTesting Instructions
ScrollLockcontinues to function as expected