Controller: @phase2/outline-controller-resize-controller
#393
Conversation
shaal
requested review from
himerus,
mkochendorfer,
ayanapowell,
glitchgirl and
bardleb
|
✅ Deploy Preview for outlinejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
himerus
added
Type: Controller Suggestion
himerus
removed
the
CANNOT MERGE
himerus
changed the title
@phase2/outline-controller-resize-controller
WalkthroughThe update introduces a new utility module named Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
packages/controllers/resize-controller/package.jsonis excluded by:!**/*.jsonpackages/controllers/resize-controller/tsconfig.build.jsonis excluded by:!**/*.json
Files selected for processing (3)
- packages/controllers/resize-controller/README.md (1 hunks)
- packages/controllers/resize-controller/index.ts (1 hunks)
- packages/controllers/resize-controller/src/resize-controller.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/controllers/resize-controller/index.ts
Additional comments: 2
packages/controllers/resize-controller/README.md (1)
- 29-94: API reference and example usage accurately reflect the implementation details and capabilities of the ResizeController class.
packages/controllers/resize-controller/src/resize-controller.ts (1)
- 11-29: The debounce function implementation is correct and follows best practices for debouncing.
| You can install the `resize-controller` package using yarn: | ||
|
|
||
| ``` | ||
| yarn add -D @phase2/resize-controller |
There was a problem hiding this comment.
Incorrect package name in the yarn add command.
- yarn add -D @phase2/resize-controller
+ yarn add -D @phase2/outline-controller-resize-controllerCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| yarn add -D @phase2/resize-controller | |
| yarn add -D @phase2/outline-controller-resize-controller |
| 1. Import the necessary classes and functions from the `lit` package: | ||
|
|
||
| ```javascript | ||
| import { ResizeController } from 'resize-controller'; |
There was a problem hiding this comment.
Incorrect import statement, does not match the actual package name.
- import { ResizeController } from 'resize-controller';
+ import { ResizeController } from '@phase2/outline-controller-resize-controller';Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| import { ResizeController } from 'resize-controller'; | |
| import { ResizeController } from '@phase2/outline-controller-resize-controller'; |
| */ | ||
| initializeBreakpointsRangeType() { | ||
| // This will allow create an additional breakpoint from the last custom breakpoint to 100000 | ||
| this.options.breakpoints?.push(100000); |
There was a problem hiding this comment.
Modifying the options object directly by pushing to breakpoints array can lead to unexpected side effects.
- this.options.breakpoints?.push(100000);
+ const extendedBreakpoints = [...this.options.breakpoints, 100000];
+ this.options = { ...this.options, breakpoints: extendedBreakpoints };Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| this.options.breakpoints?.push(100000); | |
| const extendedBreakpoints = [...this.options.breakpoints, 100000]; | |
| this.options = { ...this.options, breakpoints: extendedBreakpoints }; |
| if (!this.host.style.display) { | ||
| // adding `display: block` to :host of component | ||
| this.host.style.setProperty( | ||
| 'display', | ||
| 'var(--style-added-by-resize-controller, block)' | ||
| ); |
There was a problem hiding this comment.
Setting display: block directly on the host element may not be appropriate for all components and could override existing styles.
Consider using a more flexible approach or documenting this behavior clearly to inform users.
| // Create a new ResizeObserver and pass in the function to be called when the element is resized | ||
| this.resizeObserver = new ResizeObserver( | ||
| (entries: ResizeObserverEntry[]) => { | ||
| // Create a debounced version of the onElementResize function | ||
| debounce( | ||
| this.onElementResize.bind(this), | ||
| this.options.debounce | ||
| )(entries); | ||
| } | ||
| ); |
There was a problem hiding this comment.
The debounced function is recreated on every resize event, which negates the purpose of debouncing.
Move the debounced function creation outside of the ResizeObserver callback to ensure it's created only once.
+ const debouncedOnElementResize = debounce(
+ this.onElementResize.bind(this),
+ this.options.debounce
+ );
this.resizeObserver = new ResizeObserver(
(entries: ResizeObserverEntry[]) => {
- debounce(
- this.onElementResize.bind(this),
- this.options.debounce
- )(entries);
+ debouncedOnElementResize(entries);
}
);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Create a new ResizeObserver and pass in the function to be called when the element is resized | |
| this.resizeObserver = new ResizeObserver( | |
| (entries: ResizeObserverEntry[]) => { | |
| // Create a debounced version of the onElementResize function | |
| debounce( | |
| this.onElementResize.bind(this), | |
| this.options.debounce | |
| )(entries); | |
| } | |
| ); | |
| // Create a new ResizeObserver and pass in the function to be called when the element is resized | |
| const debouncedOnElementResize = debounce( | |
| this.onElementResize.bind(this), | |
| this.options.debounce | |
| ); | |
| this.resizeObserver = new ResizeObserver( | |
| (entries: ResizeObserverEntry[]) => { | |
| debouncedOnElementResize(entries); | |
| } | |
| ); |
| } else if (this.currentComponentWidth === 0) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `resize-controller: No width detected in <${this.host.localName}>. Please confirm it has display: block` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Logging a warning to the console for a width of 0 may not always indicate an issue, especially for initially hidden elements.
Consider a more robust way to handle or document this scenario to avoid confusion.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/controllers/resize-controller/README.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/controllers/resize-controller/README.md
|
this needs to be updated @shaal |
Testing
Description
The new resize controller includes multiple improvements over previous attempts
This resize-controller has been extensively tested by multiple projects.
Component width is being tracked by the Browser API's resize observer
Debounce during resize to reduce amount of events
Only when component width moves over from one breakpoint range to another, component update is called.
During component render, only current breakpoint range variable is read (no function is being called)
Provides the ability to separate the element tracking width from the element that will be updated (useful when we need to synchronize all items in a container to switch together at the same breakpoint).
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist
Summary by CodeRabbit
ResizeControllerutility for monitoring and reacting to size changes in web components, enabling responsive design adjustments.