Open
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host (limel-masonry-layout)
participant RO as ResizeObserver
participant MO as MutationObserver
participant Scheduler as Debounced rAF Scheduler
participant Layout as Layout Engine
participant Item as Slotted Item(s)
Host->>RO: observe host size
Host->>MO: observe child-list changes
RO-->>Scheduler: size change event
MO-->>Scheduler: DOM change event
Scheduler-->>Layout: schedule layout pass
Layout->>Item: read dimensions (getBoundingClientRect)
Layout->>Layout: compute columnCount & positions
Layout->>Item: apply inline styles (position/left/top/width)
Layout->>Host: set container height, add `.is-laid-out`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3986/ |
Kiarokh
force-pushed
the
masonry-layout
branch
2 times, most recently
from
e3620b7 to
82135fb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/masonry-layout/examples/masonry-layout-basic.tsx`:
- Around line 22-35: The items property is never reassigned so mark it readonly
to satisfy SonarCloud: change the declaration of items (symbol: items) to a
readonly property (e.g., private readonly items or private readonly items:
ReadonlyArray<{title: string; height: number}>), keeping the current array
literal; update any consumer code if it relied on mutating items (use copies for
mutations) so the array and its elements are treated as immutable.
In `@src/components/masonry-layout/examples/masonry-layout-images.tsx`:
- Line 47: The img element is missing explicit dimensions causing layoutItems()
to measure zero-height cards before images load; update the <img> in
masonry-layout-images.tsx to include the known image.width and image.height (use
image.width and image.height as the width and height attributes) so the initial
masonry layout uses the correct intrinsic sizes and prevents shifting as images
load.
In `@src/components/masonry-layout/examples/masonry-layout-ordered.tsx`:
- Around line 73-81: The method handleChangeTab is defined as an arrow property
but never reassigned; mark it readonly to reflect immutability and prevent
accidental reassignment. Update the declaration of handleChangeTab (the arrow
function that takes event: CustomEvent<Tab> and maps this.tabs) to include the
readonly modifier so the property cannot be reassigned while leaving its
implementation unchanged.
- Around line 35-45: The items array is never reassigned so mark it readonly to
communicate immutability and enable TypeScript checks; update the declaration of
the private field named items (in the component/class containing items) to use
the readonly modifier (e.g., private readonly items) and, if desired, type it as
a readonly tuple/array (ReadonlyArray or readonly { label: string; height:
number }[]) to prevent mutation of elements.
In `@src/components/masonry-layout/masonry-layout.tsx`:
- Line 131: Remove the trailing commas in multiline parameter/argument lists in
this file (e.g., change "rootFontSize: number," to "rootFontSize: number" and
remove the trailing commas at the other reported occurrences). Locate the
parameter and call argument lists in
src/components/masonry-layout/masonry-layout.tsx (search for symbols like
rootFontSize and other multiline parameter/argument blocks) and delete the
trailing commas so the lists end without a comma; re-run the linter to confirm
the file now passes.
- Around line 53-67: The layout can go stale when CSS variables like
--masonry-layout-gap or --masonry-layout-min-column-width change without a host
resize; inside componentDidLoad add a lightweight watcher that reads
getComputedStyle(this.host).getPropertyValue for those two variables, store
their initial values, and on a short interval (or requestAnimationFrame loop)
compare current values to the stored ones and call observeItemSizes() and
scheduleLayout() when they differ; ensure the watcher is cleaned up (clearing
the interval/raf) when the component is torn down so you avoid leaks.
- Around line 38-39: The ordered Prop doesn't trigger recomputing item positions
when changed at runtime; add a watcher for the ordered property (use
`@Watch`('ordered') or equivalent) that calls the component's existing layout
routine (e.g., relayout(), layoutChildren(), computePositions(), or whatever
method currently sets children's inline left/top) so toggling ordered forces a
recompute and repositions children immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56f9ef91-cfd2-4c84-b21f-36dc6ee49a28
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (10)
src/components/masonry-layout/examples/masonry-layout-basic.scsssrc/components/masonry-layout/examples/masonry-layout-basic.tsxsrc/components/masonry-layout/examples/masonry-layout-images.scsssrc/components/masonry-layout/examples/masonry-layout-images.tsxsrc/components/masonry-layout/examples/masonry-layout-ordered.scsssrc/components/masonry-layout/examples/masonry-layout-ordered.tsxsrc/components/masonry-layout/masonry-layout.scsssrc/components/masonry-layout/masonry-layout.tsxsrc/examples/whats-new/example-whats-new.scsssrc/examples/whats-new/example-whats-new.tsx
src/components/masonry-layout/examples/masonry-layout-basic.tsx
Outdated
Show resolved
Hide resolved
src/components/masonry-layout/examples/masonry-layout-images.tsx
Outdated
Show resolved
Hide resolved
src/components/masonry-layout/examples/masonry-layout-ordered.tsx
Outdated
Show resolved
Hide resolved
src/components/masonry-layout/examples/masonry-layout-ordered.tsx
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/masonry-layout/examples/masonry-layout-images.tsx`:
- Around line 47-53: The img elements in masonry-layout-images.tsx use
alt={`Photo ${image.id}`} which is not meaningful for assistive tech; change the
alt handling in the JSX where src and image (image.id, image.width,
image.height) are used so decorative/demo images use alt="" instead, or if
descriptive text is available add a real description (e.g., image.description or
image.title) instead of the numeric id; update the img tag's alt prop
accordingly to either an empty string for purely decorative/demo images or a
meaningful text property from the image data.
In `@src/components/masonry-layout/examples/masonry-layout-ordered.tsx`:
- Around line 43-64: The render() function currently returns an array of two
sibling elements which violates StencilJS guidelines; update the component to
import Host from '@stencil/core' and wrap the two siblings (the
<limel-example-controls> block and the <section> containing
<limel-masonry-layout ordered={this.ordered}>) inside a single <Host> element so
render() returns one root node; ensure any references like this.ordered,
this.setOrdered, and this.items remain unchanged and that Host is added to the
component imports.
In `@src/components/masonry-layout/masonry-layout.tsx`:
- Around line 69-73: The mutation handler attached in the constructor using
this.mutationObserver currently calls observeItemSizes() and scheduleLayout()
but does not clear inline geometry on nodes removed from the masonry; update the
MutationObserver callback (the function that calls this.observeItemSizes() and
this.scheduleLayout()) to iterate mutation.removedNodes and for each removed
element remove/reset the styles set by positionItems (clear position, left, top,
width, animationDelay) so reparented or reused elements do not retain stale
absolute positioning; reference the mutationObserver, observeItemSizes,
scheduleLayout, positionItems, and removedNodes when making the change.
- Around line 134-149: The getCssPropertyInPx function currently treats any
non-'px' value as 'rem' which yields NaN or wrong sizes for units like calc(),
%, em, etc.; update getCssPropertyInPx to (1) read the raw value from
getComputedStyle(this.host).getPropertyValue(property).trim(), (2) if value
endsWith('px') parse and return the px number, (3) if value endsWith('rem')
parse the number and return number * rootFontSize, (4) otherwise attempt to
parse a plain numeric string and treat it as px, and if parsing yields NaN or
the unit is unsupported (calc, %, em, etc.) fall back to parsing the provided
fallback string (apply the same px/rem handling to fallback) or return a safe
default (e.g., 0) to avoid producing NaN for layout calculations; update
references in getCssPropertyInPx to ensure fallback handling uses the same
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c440a4ef-5d81-4d64-bd1c-0bb8586e2a2e
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (5)
src/components/masonry-layout/examples/masonry-layout-basic.tsxsrc/components/masonry-layout/examples/masonry-layout-images.tsxsrc/components/masonry-layout/examples/masonry-layout-ordered.scsssrc/components/masonry-layout/examples/masonry-layout-ordered.tsxsrc/components/masonry-layout/masonry-layout.tsx
src/components/masonry-layout/examples/masonry-layout-ordered.tsx
Outdated
Show resolved
Hide resolved
jgroth
reviewed
Apr 1, 2026
Comment on lines
+18
to
+21
| * :::note | ||
| * This component has `shadow: false`, so consumers can treat it as | ||
| * an internal part of their UI and freely style the slotted elements. | ||
| * ::: |
Contributor
There was a problem hiding this comment.
The shadow DOM does not affect slotted children since they belong to the light DOM, i.e. the consumer has full control over their styling. So this note can be removed
Contributor
Author
There was a problem hiding this comment.
what I'm trying to say is that the children of limel-masonry-layout can be treated/ styled by the consumer of limel-masonry-layout. It is as if the consumer has a div, wrapping the children. Since limel-masonry-layout has shadow set to false. It's like a generic div in the consumer.
So i improved the docs here: e8d5b89
Contributor
There was a problem hiding this comment.
Yeah but it does not matter whether we set shadow to trueor false, the consumer would have control of their components no matter what. The shadow DOM only protects what we internally render inside our own components. I would say setting shadow to false here creates a risk that the consumer could mess up the layout of the layout component since it's not protected by the shadow DOM
Contributor
Author
There was a problem hiding this comment.
Aha. I see what you mean. Since they are slotted it doesn't matter. Ok, I'll change
Contributor
Author
There was a problem hiding this comment.
I'm gonna change to shadow: true and push a fixup
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/masonry-layout/masonry-layout.tsx`:
- Around line 54-82: The component tears down observers in disconnectedCallback
but only creates them in componentDidLoad, so if the element is removed and
later reinserted observers never get reattached; fix by moving observer setup
into a lifecycle that runs on every attachment (e.g., implement
connectedCallback or extract a setupObservers method called from both
componentDidLoad and connectedCallback) and ensure the same
symbols—resizeObserver, itemResizeObserver, mutationObserver—are reinitialized
and start observing (this.host and items via observeItemSizes) there, while
keeping disconnectedCallback to disconnect and cancel pendingLayoutFrame as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25dceacb-92ba-48b6-9c8f-58bfcc40e3ca
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (10)
src/components/masonry-layout/examples/masonry-layout-basic.scsssrc/components/masonry-layout/examples/masonry-layout-basic.tsxsrc/components/masonry-layout/examples/masonry-layout-images.scsssrc/components/masonry-layout/examples/masonry-layout-images.tsxsrc/components/masonry-layout/examples/masonry-layout-ordered.scsssrc/components/masonry-layout/examples/masonry-layout-ordered.tsxsrc/components/masonry-layout/masonry-layout.scsssrc/components/masonry-layout/masonry-layout.tsxsrc/examples/whats-new/example-whats-new.scsssrc/examples/whats-new/example-whats-new.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/masonry-layout/masonry-layout.tsx`:
- Around line 260-262: The staggered transitionDelay is being set on
consumer-owned slotted elements in the block guarded by this.hasRendered (the
line setting item.style.transitionDelay = `${index * 20}ms`), which can
undesirably delay other consumer transitions; modify the logic so the inline
transitionDelay is removed once the component's opacity transition finishes
(attach a one-time 'transitionend' handler for the opacity property on the same
element to clear item.style.transitionDelay), or instead set a component-scoped
CSS custom property (e.g., --masonry-transition-delay) on the host and use that
in the component's CSS so you don't mutate consumer element inline styles;
implement one of these in the code paths around the this.hasRendered block and
ensure the cleanup handler is removed after firing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cdda356b-5f85-47ff-91ab-50e25fda75c9
📒 Files selected for processing (1)
src/components/masonry-layout/masonry-layout.tsx
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
fix #3985
Summary by CodeRabbit
New Features
Documentation
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: