Table card search and scopes#182
Open
Sobyt483 wants to merge 5 commits into
Open
Conversation
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
|
Warning Review limit reached
More reviews will be available in 52 minutes and 30 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIntroduces a new ChangesTableCardSearch extraction and search/scope API
Sequence Diagram(s)sequenceDiagram
participant Host as Host Component
participant DTC as DeclarativeTableCard
participant TCS as TableCardSearch
participant UI5 as ui5-search
rect rgba(100, 149, 237, 0.5)
Note over DTC,TCS: Initialization
Host->>DTC: config() with searchConfig
DTC->>TCS: [searchConfig]="searchConfig()"
end
rect rgba(144, 238, 144, 0.5)
Note over TCS,UI5: User input interaction
UI5->>TCS: input event → onSearchInput()
TCS->>TCS: FormControl.setValue()
TCS->>TCS: debounce 300ms
TCS-->>DTC: searchChanged { value, scope? }
DTC-->>Host: searchChanged output
end
rect rgba(255, 165, 0, 0.5)
Note over TCS,UI5: Submit and scope change
UI5->>TCS: search-submit → onSearchSubmit()
TCS-->>DTC: searchSubmit { value, scope? }
UI5->>TCS: scope-change → onSearchScopeChange()
TCS->>TCS: update activeScope
TCS-->>DTC: scopeChanged { value, scope? }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
projects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.ts (1)
703-710: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert delegated child output forwarding, not just emitter existence.
These tests pass even if the template bindings for
searchSubmitandscopeChangedare removed. Emit from the renderedmfp-table-card-searchtest node and assert the parent outputs receive the same payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.ts` around lines 703 - 710, The tests for exposes searchSubmit output and exposes scopeChanged output only verify that the emit functions exist on the component outputs, but do not test that the parent component actually receives and forwards the events from the child mfp-table-card-search component. Replace these tests to emit events from the rendered child component in the template, subscribe to or spy on the parent component's searchSubmit and scopeChanged outputs, verify that the parent outputs receive the same event payloads that were emitted from the child, ensuring true integration between parent and child component outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts`:
- Around line 480-508: The DeclarativeTableCardSearchStory component needs to be
updated to follow Angular component guidelines. Add `standalone: true` and
`changeDetection: ChangeDetectionStrategy.OnPush` to the component decorator
configuration. Replace the `@Input`() decorators for the config and resources
properties with signal-based inputs using the input() function instead. Import
ChangeDetectionStrategy from '`@angular/core`' and the input function from
'`@angular/core`' to enable these changes.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.html`:
- Around line 6-7: The scopeValue binding in the ui5-search component should be
connected to the component's live activeScope() method instead of
searchConfig().scopeValue to ensure the scope selection persists when the search
element is recreated during collapse/re-expand cycles. Update the [scopeValue]
binding property in the ui5-search template to use activeScope() as its source,
and apply this same change to the other occurrence mentioned at lines 27-28 to
keep both search element instances synchronized with the internal scope state.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.spec.ts`:
- Around line 59-60: The ESLint suppressions for
`@typescript-eslint/no-explicit-any` at lines 59, 66, 74, 83, 85, 93, 353, 366,
380, 392, and 395 are missing required explanatory comments and TODO statements
per coding guidelines. Either add a documented rationale comment above each
suppression explaining why the any cast is necessary and include a TODO
indicating when it should be removed, or refactor the test assertions to rely on
public component outputs and DOM assertions instead of white-box access to
internal state properties like searchExpanded(), searchCollapsing(),
searchControl, and searchState().
- Line 4: In the table-card-search.component.spec.ts file, remove
NO_ERRORS_SCHEMA from the import statement at the top of the file where
CUSTOM_ELEMENTS_SCHEMA and NO_ERRORS_SCHEMA are imported together from
`@angular/core`. Then, locate the TestBed configuration (typically in a beforeEach
setup function) where schemas array is defined and remove NO_ERRORS_SCHEMA from
that array, leaving only CUSTOM_ELEMENTS_SCHEMA. This aligns the test
configuration with the component's own schema configuration and respects the
strictTemplates Angular setting.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts`:
- Around line 55-77: The searchControl form control is initialized to an empty
string and never updated when the searchConfig signal changes, breaking the
ability for parent components to control the search value through
searchConfig.value. Update the existing effect in the constructor that currently
only syncs activeScope to also sync the searchConfig().value to the
searchControl. Use patchValue on searchControl to update its value when
searchConfig().value changes, ensuring that parent-driven value control works as
intended.
---
Nitpick comments:
In
`@projects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.ts`:
- Around line 703-710: The tests for exposes searchSubmit output and exposes
scopeChanged output only verify that the emit functions exist on the component
outputs, but do not test that the parent component actually receives and
forwards the events from the child mfp-table-card-search component. Replace
these tests to emit events from the rendered child component in the template,
subscribe to or spy on the parent component's searchSubmit and scopeChanged
outputs, verify that the parent outputs receive the same event payloads that
were emitted from the child, ensuring true integration between parent and child
component outputs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8962f7b9-8b0e-4e99-9194-62157353bda2
📒 Files selected for processing (15)
docs/declarative-table-card.mdprojects/ngx/declarative-ui/stories/declarative-table-card.stories.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.htmlprojects/ngx/declarative-ui/table-card/declarative-table-card.component.scssprojects/ngx/declarative-ui/table-card/declarative-table-card.component.spec.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.tsprojects/ngx/declarative-ui/table-card/index.tsprojects/ngx/declarative-ui/table-card/models/configs.tsprojects/ngx/declarative-ui/table-card/models/search-config.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.htmlprojects/ngx/declarative-ui/table-card/search/table-card-search.component.scssprojects/ngx/declarative-ui/table-card/search/table-card-search.component.spec.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.tsprojects/ngx/declarative-ui/table/models/index.tsprojects/ngx/declarative-ui/table/models/table-config.ts
💤 Files with no reviewable changes (1)
- projects/ngx/declarative-ui/table-card/declarative-table-card.component.scss
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Member
|
If we go with this implementation then:
|
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Contributor
Author
@gkrajniak live inside ui-definition
Done |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts (1)
13-13: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign the story wrapper with Angular component rules (
standalone, OnPush, signal inputs).
DeclarativeTableCardCreateStorystill uses@Input()+OnInitinitialization, and the shown decorator does not includestandalone: true/changeDetection: ChangeDetectionStrategy.OnPush. Please migrate to signal inputs (input()) and initialize derived state viaeffect()instead ofngOnInit.Suggested fix
-import { Component, Input, OnInit } from '`@angular/core`'; +import { ChangeDetectionStrategy, Component, effect, input } from '`@angular/core`'; @@ `@Component`({ + standalone: true, + changeDetection: ChangeDetectionStrategy.OnPush, template: ` @@ }) -class DeclarativeTableCardCreateStory implements OnInit { - `@Input`() config!: TableCardConfig; - `@Input`() resources: GenericResource[] = []; +class DeclarativeTableCardCreateStory { + config = input.required<TableCardConfig>(); + resources = input<GenericResource[]>([]); @@ - ngOnInit(): void { - this.searchTerm = this.config?.searchConfig?.value ?? ''; - this.activeScope = this.config?.searchConfig?.scopeValue; - } + constructor() { + effect(() => { + const searchConfig = this.config().searchConfig; + this.searchTerm = searchConfig?.value ?? ''; + this.activeScope = searchConfig?.scopeValue; + }); + } @@ - if (!sc) return this.resources; + if (!sc) return this.resources(); @@ - let result = this.resources as Pod[]; + let result = this.resources() as Pod[];As per coding guidelines: "
**/*.{ts,tsx}: Use standalone components withstandalone: trueconfiguration", "Use signal-based APIs:input(),output(),model(),computed(),effect()", and "Use OnPush change detection on all components".Also applies to: 168-170, 176-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts` at line 13, The DeclarativeTableCardCreateStory component uses outdated Angular patterns and needs to be modernized. Replace all `@Input`() decorators with signal input() calls, remove the OnInit interface from the class implements clause, and delete the ngOnInit method. Instead, use effect() to handle any derived state initialization that was previously in ngOnInit. Update the component decorator to include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush. Make sure to import the necessary signal functions (input, effect) from `@angular/core` and import ChangeDetectionStrategy from `@angular/core`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.html`:
- Around line 13-14: The `@for` loop tracking key in the scopes iteration uses
s.value, which is optional and can cause collisions when multiple items have
undefined values, leading to incorrect DOM diffing. Replace the track expression
from track s.value to track $index to use the loop index as a collision-safe
tracking key, ensuring each DOM element maintains its identity correctly even
when value properties are undefined or duplicated.
In
`@projects/ngx/declarative-ui/table-card/search/table-card-search.component.ts`:
- Around line 69-72: The fixSelectWidth() workaround is currently applied only
once on initial render within the setTimeout block. When searchConfig().scopes
is updated or changed after the component initializes, the workaround does not
run again, causing long scope labels to truncate. Add a reactive effect or
watcher that monitors changes to searchConfig().scopes and triggers
fixSelectWidth() whenever the scopes change, ensuring the select width
adjustment is applied every time the scopes are updated.
---
Duplicate comments:
In `@projects/ngx/declarative-ui/stories/declarative-table-card.stories.ts`:
- Line 13: The DeclarativeTableCardCreateStory component uses outdated Angular
patterns and needs to be modernized. Replace all `@Input`() decorators with signal
input() calls, remove the OnInit interface from the class implements clause, and
delete the ngOnInit method. Instead, use effect() to handle any derived state
initialization that was previously in ngOnInit. Update the component decorator
to include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush.
Make sure to import the necessary signal functions (input, effect) from
`@angular/core` and import ChangeDetectionStrategy from `@angular/core`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67fad9bf-0490-4bc5-b414-2b1238b79e66
📒 Files selected for processing (11)
projects/ngx/declarative-ui/stories/declarative-table-card.stories.tsprojects/ngx/declarative-ui/table-card/declarative-table-card.component.htmlprojects/ngx/declarative-ui/table-card/declarative-table-card.component.scssprojects/ngx/declarative-ui/table-card/declarative-table-card.component.tsprojects/ngx/declarative-ui/table-card/models/configs.tsprojects/ngx/declarative-ui/table-card/models/search-config.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.htmlprojects/ngx/declarative-ui/table-card/search/table-card-search.component.scssprojects/ngx/declarative-ui/table-card/search/table-card-search.component.spec.tsprojects/ngx/declarative-ui/table-card/search/table-card-search.component.tsprojects/ngx/declarative-ui/table/models/table-config.ts
💤 Files with no reviewable changes (4)
- projects/ngx/declarative-ui/table-card/models/search-config.ts
- projects/ngx/declarative-ui/table-card/declarative-table-card.component.html
- projects/ngx/declarative-ui/table-card/models/configs.ts
- projects/ngx/declarative-ui/table-card/declarative-table-card.component.ts
✅ Files skipped from review due to trivial changes (1)
- projects/ngx/declarative-ui/table-card/search/table-card-search.component.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/ngx/declarative-ui/table/models/table-config.ts
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Member
|
the idea behind the predefined filters is to be able to build search queries, fq: (member:"C5343390") AND accountRole:"Project" that's why there was initial proposal to use in the event the it the current solution the export interface FieldFielterDefinition/Scope {
label: string;
property?: string | string[];
value: string;
}once having it the search query can be better produce: fq: property=value&qtext=value |
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.
Known limitations: there is currently no way to natively extend the width of the scope select. Long labels get truncated. A DOM manipulation workaround can be applied inside the component, but it's fragile.
I have raised an issue in the ui5-webcomponents repository:
UI5/webcomponents#13719
Summary by CodeRabbit
Release Notes
New Features
Refactor
Documentation