popup separated #736
popup separated #736
Conversation
|
@CodeRabbit summary |
✅ Actions performedSummary regeneration triggered. |
WalkthroughThis change refactors the filter UI system by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FilterButton
participant FilterPopup
participant FilterPanel
participant Grid
User->>FilterButton: Clicks filter button
FilterButton->>FilterPopup: show(x, y)
FilterPopup->>FilterPanel: show()
Note right of FilterPanel: User interacts with filter UI
User->>FilterPopup: Clicks outside (if enabled)
FilterPopup->>FilterPopup: Detects outside click
FilterPopup->>FilterPopup: hide()
FilterPopup->>FilterPanel: hide()
FilterPopup-->>Grid: Emits "close" event
Possibly related PRs
Suggested labels
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/plugins/filter/filter.panel.tsx (1)
417-488: 🛠️ Refactor suggestionAdd key properties to elements in iterables
The renderContent method returns an array of JSX elements without key properties, which may cause rendering optimization issues.
- return [ - <slot slot="header" />, - this.changes.extraContent?.(this.changes) || '', + return [ + <slot key="header-slot" slot="header" />, + <div key="extra-content">{this.changes.extraContent?.(this.changes) || ''}</div>, this.changes?.hideDefaultFilters !== true ? ( [ - <label>{capts.title}</label>, - <div class="filter-holder">{this.getFilterItemsList()}</div>, - <div class="add-filter"> + <label key="filter-label">{capts.title}</label>, + <div key="filter-holder" class="filter-holder">{this.getFilterItemsList()}</div>, + <div key="add-filter" class="add-filter"> <select id={FILTER_ID} class="select-css" onChange={e => this.onAddNewFilter(e)} > {this.renderSelectOptions(this.currentFilterType)} </select> </div> ] ) : '', - <slot />, + <slot key="default-slot" />, <div class="filter-actions"> {this.disableDynamicFiltering && [ <button + key="save-button" id="revo-button-save" aria-label="save" class="revo-button green" onClick={() => this.onSave()} > {capts.save} </button>, <button + key="cancel-button-1" id="revo-button-ok" aria-label="ok" class="revo-button green" onClick={() => this.onCancel()} > {capts.cancel} </button>, ]} {!this.disableDynamicFiltering && [ <button + key="ok-button" id="revo-button-ok" aria-label="ok" class="revo-button green" onClick={() => this.onCancel()} > {capts.ok} </button>, <button + key="reset-button" id="revo-button-reset" aria-label="reset" class="revo-button outline" onClick={() => this.onReset()} > {capts.reset} </button>, ]} </div>, - <slot slot="footer" />]; + <slot key="footer-slot" slot="footer" />];🧰 Tools
🪛 Biome (1.9.4)
[error] 428-428: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 447-447: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 448-448: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 487-487: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 433-433: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 434-434: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 435-435: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 450-455: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 458-463: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 468-473: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 477-482: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🧹 Nitpick comments (2)
src/plugins/filter/filter.plugin.tsx (2)
63-64: References added for popup and panel.
ThepopupRefandpanelReffields allow you to manipulate the popup and panel elements directly. Ensure they are properly cleaned up or nulled out if there’s any teardown logic required in the future.
158-163: Suggestion to add dedicated "hide" method on panel.
Currently, the panel is closed by callingshow(undefined). While valid, a dedicatedhide()method on the panel might clarify the intended action.- if (this.panelRef) { - this.panelRef.show(undefined); - } + if (this.panelRef?.hide) { + this.panelRef.hide(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs(1 hunks)src/components.d.ts(7 hunks)src/plugins/filter/filter.panel.tsx(5 hunks)src/plugins/filter/filter.plugin.tsx(5 hunks)src/plugins/filter/filter.popup.tsx(1 hunks)src/plugins/filter/filter.style.scss(1 hunks)src/types/events.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components.d.ts (1)
src/plugins/filter/filter.types.ts (1)
ShowData(160-173)
🪛 Biome (1.9.4)
src/plugins/filter/filter.plugin.tsx
[error] 92-96: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
src/plugins/filter/filter.panel.tsx
[error] 428-428: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 447-447: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 487-487: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
src/components.d.ts
[error] 1049-1049: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (24)
docs (1)
1-1:❓ Verification inconclusive
Documentation subproject reference updated.
The documentation subproject reference has been updated to commit
4ffe7c9ac2a849d04231551308fd462b366b3c19. This update is necessary to align the documentation with the code changes, particularly the introduction of the newrevogr-filter-popupcomponent and related filter UI refactoring.Please ensure that the documentation properly reflects the new component structure and API changes made in this PR. Specifically, check that:
- The new
revogr-filter-popupcomponent is documented- The updated filter plugin API is correctly described
- Any changes to event types and interactions are properly documented
🏁 Script executed:
#!/bin/bash # Check if the documentation has been updated to include references to the new filter popup component echo "Checking docs for references to the new filter popup component..." cd docs git show -p | grep -i "filter-popup" echo "Done checking docs."Length of output: 631
The documentation shows clear evidence that the new revogr-filter-popup component is referenced in multiple locations, including the component’s type definitions and HTML usage. However, please take an additional manual look to confirm that the updated filter plugin API description and any new event type or interaction changes have been comprehensively covered in the docs.
Next Steps for Verification:
- Confirm that the documentation clearly explains the new filter plugin API changes.
- Verify that any changes to event types and interactions are also described.
src/types/events.ts (2)
60-60: LGTM: Event type addition for popup closingAdding the 'close' event supports the new popup component's ability to notify when it's closed due to outside clicks.
170-170: LGTM: Event mapping added to constantsConsistent update to the REVOGRID_EVENTS map to include the new 'close' event type.
src/plugins/filter/filter.style.scss (2)
1-18: LGTM: CSS reorganization for popup componentThe styles have been appropriately moved from panel to popup, maintaining the same visual styling while supporting the component separation. This CSS restructuring properly encapsulates the positioning, visibility, and container styles in the new popup component.
20-23: LGTM: Simplified panel stylingThe panel component now has simplified styles focused on display and width, which aligns with its reduced responsibilities after the refactoring.
src/plugins/filter/filter.popup.tsx (6)
1-12: LGTM: Well-structured importsGood organization of imports for Stencil.js decorators and lifecycle hooks.
14-27: LGTM: Component setup and property definitionsThe component is properly set up with state variables for visibility and position, and exposes the necessary events and properties.
29-39: LGTM: Show/hide methods implementationThe show and hide methods provide a clean API for controlling the popup's visibility and position.
41-58: LGTM: Outside click detection logicThe global mousedown listener correctly implements outside click detection and emits the close event when appropriate.
60-77: LGTM: Position auto-correction implementationThe auto-correction logic ensures the popup stays within the grid boundaries, improving usability.
79-95: LGTM: Conditional rendering based on visibilityThe render method appropriately handles visibility state and applies inline positioning styles when visible.
src/plugins/filter/filter.panel.tsx (3)
17-17: LGTM: Updated importsImport statement now correctly imports only the button components needed without including the
isFilterBtnhelper which is now used in the popup component.
75-84: LGTM: Simplified show methodThe show method has been refactored to focus on setting state and returning rendered content, delegating positioning logic to the parent popup component.
490-496: LGTM: Simplified render methodThe render method has been simplified to use the new renderContent method, which improves code organization.
src/plugins/filter/filter.plugin.tsx (7)
36-37: Constants look good.
These new constantsFILTER_PANELandFILTER_POPUPare simple and descriptive, and they align with the refactored components.
88-88: Filter logic is clear.
Filtering out any previous panel or popup references fromregisterVNodebefore adding new ones looks fine.
92-96: Potential false-positive lint warning about missing key property.
A static analysis hint flags missing keys in an iterable, but this appears to be Stencil code rather than React. If this is not a React environment or you don't render multiple instances of this element in a loop, that warning can be safely ignored. Otherwise, consider adding a unique key prop if these nodes are indeed iterated.🧰 Tools
🪛 Biome (1.9.4)
[error] 92-96: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
102-102: Configuration binding is consistent.
PassingdisableDynamicFilteringdirectly fromconfigis straightforward, ensuring your panel is aware of dynamic filtering behavior.
165-171: Good approach to toggle panel & popup visibility.
Thebeforeshowmethod logically opens or closes both the popup and panel based on the presence of data. This centralizes control nicely.
175-175: Returning extra content is fine.
extraHyperContentis optional, so these lines make sense. Just ensure null or undefined is handled gracefully within the content consumer.
256-256: Null/undefined check is straightforward.
The guard clause cleanly exits if relevant references are missing. This is a simple and safe approach.src/components.d.ts (3)
452-453: Clearer return type for “show” method.
Theshowmethod returningPromise<any[] | null>is acceptable, but documenting its return usage might help future maintainers.
454-457: New popup interface.
DefiningRevogrFilterPopupis a good abstraction for popup management. The doc comment helps keep code self-documenting.
768-771: Custom event interface matches existing pattern.
RevogrFilterPopupCustomEvent<T>follows the same structure as other custom event interfaces, ensuring consistency for typed events.
| "close": void; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace “void” with “undefined” to satisfy lint.
Static analysis warns that void can be confusing here. Updating "close": undefined; aligns better with typical TypeScript usage:
- "close": void;
+ "close": undefined;📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "close": void; | |
| } | |
| "close": undefined; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1049-1049: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Summary by CodeRabbit
New Features
Refactor
Documentation
Bug Fixes