upgrade and fix ember-template-lint issues #1499
Conversation
WalkthroughThis pull request introduces several changes across various components, primarily focusing on updating action bindings to remove the Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (36)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (33)
🔇 Additional comments (3)app/components/ak-date-picker/index.stories.js (1)
The functions are using the older -function onSelectCollection(collection) {
- this.set('selected', collection.date);
-}
-
-function onSelectRange(range) {
- this.set('selected', range.date);
-}
-
-function onSelectDay(day) {
- this.set('selected', day?.date);
-}
+class DatePickerStory {
+ @tracked selected;
+
+ onSelectCollection = (collection) => {
+ this.selected = collection.date;
+ };
+
+ onSelectRange = (range) => {
+ this.selected = range.date;
+ };
+
+ onSelectDay = (day) => {
+ this.selected = day?.date;
+ };
+}app/components/ak-accordion/index.stories.js (2)
The functions are using the older -function handleChange() {
- this.set('accordionExpanded', !this.accordionExpanded);
-}
-
-function handleChangeId(id) {
- this.set(id, !this[id]);
-}
+class AccordionStory {
+ @tracked accordionExpanded = false;
+
+ handleChange = () => {
+ this.accordionExpanded = !this.accordionExpanded;
+ }
+
+ handleChangeId = (id) => {
+ this[id] = !this[id];
+ }
+}
The style attributes have been correctly updated to use the Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1612-fix-ember-template-lint-errors
|
| Run status |
|
| Run duration | 05m 05s |
| Commit |
|
| Committer | Avi Shah |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an apk file: 58062 |
Test Replay
Screenshots
|
|
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
6363ebc to
a5bdeb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/acceptance/breadcrumbs-test.js (1)
Line range hint
865-870: Inconsistent use of@modeland@modelsinAkLinkcomponentsIn the first
AkLink, you're using@model="1"to pass a single model, while in the secondAkLink, you're using@models={{array 2}}to pass a single model within an array. For consistency and to prevent potential issues, consider using the same attribute in both links.If the route expects a single model, use
@modelin both:<AkLink @route="authenticated.tr-b-root.parent-with-model" @model="1" data-test-linkToRootBParentWithModel1> Link to Root B - Parent With Model 1 </AkLink> -<AkLink @route="authenticated.tr-b-root.parent-with-model" @models={{array 2}} data-test-linkToRootBParentWithModel2> +<AkLink @route="authenticated.tr-b-root.parent-with-model" @model="2" data-test-linkToRootBParentWithModel2> Link to Root B - Parent With Model 2 </AkLink>If the route expects multiple models, use
@modelswith an array in both:-<AkLink @route="authenticated.tr-b-root.parent-with-model" @model="1" data-test-linkToRootBParentWithModel1> +<AkLink @route="authenticated.tr-b-root.parent-with-model" @models={{array 1}} data-test-linkToRootBParentWithModel1> Link to Root B - Parent With Model 1 </AkLink> <AkLink @route="authenticated.tr-b-root.parent-with-model" @models={{array 2}} data-test-linkToRootBParentWithModel2> Link to Root B - Parent With Model 2 </AkLink>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(15 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(1 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(1 hunks)app/components/ak-checkbox/index.stories.js(1 hunks)app/components/ak-date-picker/index.stories.js(1 hunks)app/components/ak-drawer/index.stories.js(1 hunks)app/components/ak-loader/index.hbs(1 hunks)app/components/ak-menu/index.stories.js(4 hunks)app/components/ak-modal/index.stories.js(1 hunks)app/components/ak-pagination/index.stories.js(4 hunks)app/components/ak-popover/index.stories.js(1 hunks)app/components/ak-select/index.stories.js(2 hunks)app/components/ak-tabs/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(1 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(1 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (32)
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/security/project-search-list/index.hbs
- app/components/security/analysis-list/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/upload-app/status/details/index.hbs
- app/components/organization-team/overview/index.hbs
- app/components/file-compare/header/index.hbs
- app/components/file-chart/index.hbs
- app/components/ak-loader/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/ak-autocomplete/index.stories.js
- app/components/ak-popover/index.stories.js
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/ak-drawer/index.stories.js
- package.json
- app/components/security/file-search-list/index.hbs
- app/components/ak-pagination/index.stories.js
- app/components/ak-date-picker/index.stories.js
- app/components/project-settings/header/index.hbs
- app/components/ak-checkbox-tree/index.stories.js
- app/components/ak-modal/index.stories.js
- tests/integration/components/plus-n-list-test.js
- app/components/ak-tree/index.stories.js
- app/components/ak-autocomplete/index.hbs
- .template-lintrc.js
- app/components/ak-tabs/index.stories.js
- app/components/ak-checkbox/index.stories.js
- app/components/ak-menu/index.stories.js
- tests/integration/components/partner/client-info/component-test.js
- app/components/ak-accordion/index.stories.js
- app/components/ak-select/index.stories.js
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
🔇 Additional comments (1)
tests/acceptance/breadcrumbs-test.js (1)
Line range hint 878-884: Confirm availability of this.model in ParentWithModelController
In the ParentWithModelController, you're accessing this.model.id within the breadcrumbs getter. Ensure that this.model is correctly populated to avoid potential undefined or null reference errors, especially if the model is not guaranteed to be loaded when the controller initializes.
Run the following script to check where ParentWithModelController is used and verify that the model is correctly passed:
✅ Verification successful
Let me generate additional verification scripts to check how the model is being set up in the test environment:
Let me generate one final verification script to check how the model is being passed in the test:
Based on the verification results, I can now provide the final response:
The model is correctly handled in the route and controller setup
The code shows that:
- The route is created with
createBCRoute()which implements amodel(params)hook that returns the params - The model is correctly passed via
@model="1"in the template links - The test verifies both the model access and breadcrumb generation with different model IDs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `this.model` is available in `ParentWithModelController`.
# Search for the route definition to see how the model is provided.
rg 'routeName: .+parent-with-model' -A 10
# Search for instances where `ParentWithModelController` is instantiated.
rg 'class ParentWithModelController' -A 5
Length of output: 3239
Script:
#!/bin/bash
# Search for test setup and model creation
rg "tr-b-root/parent-with-model" -B 10 -A 10
# Search for model hook or setupController implementation
ast-grep --pattern 'model() {
$$$
}'
# Search for test setup that might involve model creation
rg "setupTest|setupApplicationTest" -A 10 tests/acceptance/breadcrumbs-test.js
Length of output: 12685
Script:
#!/bin/bash
# Search for model setup and route transitions in the test
rg "LinkToRootBParentWithModel|@models={{array" -B 5 -A 5 tests/acceptance/breadcrumbs-test.js
# Search for any model hooks or data setup
rg "createBCRoute|setupRoute" -B 5 -A 5 tests/acceptance/breadcrumbs-test.js
Length of output: 12642
Deploying irenestaging with
|
| Latest commit: |
2b6703f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://58dcc138.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1612-fix-ember-template-l.irenestaging.pages.dev |
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
a5bdeb9 to
cce75db
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
cce75db to
c0dc99a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
c0dc99a to
0cd1fb9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/components/ak-menu/index.stories.js (1)
33-33: Standardize click handler implementation across templatesThe click handlers in all three templates (Default, WithIcons, and Disabled) use the
onmodifier. For consistency with other components, consider updating to use@onClick.- <AkIconButton @variant="outlined" {{on 'click' this.handleMoreClick}}> + <AkIconButton @variant="outlined" @onClick={{this.handleMoreClick}}>Also applies to: 58-58, 87-87
app/components/ak-accordion/index.stories.js (1)
59-59: Extract duplicated styles into a shared class or componentThe same styles are duplicated across multiple content blocks. Consider extracting these common styles:
- Create a CSS class:
.accordion-content { padding: 0.75rem; margin: 0.5rem; color: #ffffff; background-color: #424651; }
- Use the class instead of inline styles:
-<div class="p-3 m-2" {{style color='#ffffff' backgroundColor='#424651'}} > +<div class="accordion-content">Or create a dedicated content component:
Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(1 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(5 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(1 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (30)
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- package.json
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/upload-app/status/details/index.hbs
- app/components/project-settings/header/index.hbs
- app/components/security/file-search-list/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- tests/integration/components/partner/client-info/component-test.js
- app/components/ak-pagination/index.stories.js
- app/components/file-chart/index.hbs
- app/components/security/project-search-list/index.hbs
- app/components/file-compare/header/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/organization-team/overview/index.hbs
- app/components/ak-loader/index.hbs
- app/components/ak-autocomplete/index.hbs
- app/components/ak-checkbox/index.stories.js
- app/components/ak-date-picker/index.stories.js
- app/components/security/analysis-list/index.hbs
- app/components/ak-autocomplete/index.stories.js
- tests/integration/components/plus-n-list-test.js
- .template-lintrc.js
- app/components/ak-select/index.stories.js
- app/components/ak-modal/index.stories.js
- app/components/ak-text-field/index.stories.js
- app/components/ak-drawer/index.stories.js
- app/components/ak-tree/index.stories.js
- app/components/ak-tabs/index.stories.js
- tests/acceptance/breadcrumbs-test.js
🔇 Additional comments (8)
app/components/ak-popover/index.stories.js (3)
31-31: Maintain consistency in event handling approach
While you've updated @onBackdropClick and @closeHandler to use direct method references, the click handler still uses the on modifier. For consistency, consider updating all event handlers to the new style.
- <AkIconButton @variant="outlined" {{on 'click' this.handleMoreClick}}>
+ <AkIconButton @variant="outlined" @onClick={{this.handleMoreClick}}>43-45: LGTM! Clean event handler bindings
The direct method references for @onBackdropClick and @closeHandler follow the new pattern correctly.
66-67: LGTM! Proper action binding in Default.args
The action helper is correctly used to bind the standalone functions.
app/components/ak-checkbox-tree/index.stories.js (2)
97-103: LGTM! Clear and focused event handlers
The standalone functions are well-defined with clear responsibilities for handling expand and check events.
112-113: LGTM! Clean event handler implementation
The event handlers are properly:
- Referenced directly in the template (
@onCheckand@onExpand) - Bound using the action helper in the context
Also applies to: 116-120
app/components/ak-menu/index.stories.js (2)
18-21: LGTM! Good extraction of common arguments
The AkMenuCommonArgs object effectively reduces code duplication across story variants.
53-53: LGTM! Consistent use of common arguments
The AkMenuCommonArgs are properly spread across all story variants, maintaining consistency.
Also applies to: 82-82, 120-120
app/components/ak-accordion/index.stories.js (1)
36-42: 🛠️ Refactor suggestion
Modernize action handlers using tracked properties
The current implementation uses classic Ember patterns with this.set(). Consider modernizing these handlers:
-function handleChange() {
- this.set('accordionExpanded', !this.accordionExpanded);
-}
-
-function handleChangeId(id) {
- this.set(id, !this[id]);
-}
+// In the component class
+@tracked accordionExpanded = false;
+
+handleChange = () => {
+ this.accordionExpanded = !this.accordionExpanded;
+}
+
+handleChangeId = (id) => {
+ this[id] = !this[id];
+}Likely invalid or redundant comment.
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
0cd1fb9 to
7e4ac8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/acceptance/breadcrumbs-test.js (1)
Line range hint
865-872: Standardize model binding approachThe template uses two different approaches for passing model IDs:
@model="1"- passing a string@models={{array 2}}- passing an arrayThis inconsistency could lead to confusion and maintenance issues.
Standardize the model binding approach. If array-based binding is preferred, update both links:
-<AkLink @route="authenticated.tr-b-root.parent-with-model" @model="1" data-test-linkToRootBParentWithModel1> +<AkLink @route="authenticated.tr-b-root.parent-with-model" @models={{array 1}} data-test-linkToRootBParentWithModel1> Link to Root B - Parent With Model 1 </AkLink>
♻️ Duplicate comments (2)
app/components/ak-accordion/index.stories.js (2)
36-42: 🛠️ Refactor suggestionUse modern class syntax as previously suggested
This implementation still uses the older
this.setpattern. As mentioned in previous reviews, consider using modern class syntax:-function handleChange() { - this.set('accordionExpanded', !this.accordionExpanded); -} - -function handleChangeId(id) { - this.set(id, !this[id]); -} +handleChange = () => { + this.accordionExpanded = !this.accordionExpanded; +} + +handleChangeId = (id) => { + this[id] = !this[id]; +}
102-102: 🛠️ Refactor suggestionRemove action() helper usage as previously suggested
Multiple instances of deprecated
action()helper usage. As mentioned in previous reviews, consider using direct method references:- handleChange: action(handleChange), + handleChange() { + this.accordionExpanded = !this.accordionExpanded; + },Also applies to: 160-160, 259-259, 373-373
🧹 Nitpick comments (3)
tests/acceptance/breadcrumbs-test.js (1)
Line range hint
1-864: Consider optimizing test setupThe test file has comprehensive coverage but could benefit from optimization. Consider extracting common route configurations to a
beforeEachhook to reduce code duplication and improve maintainability.Example refactor:
module('Acceptance | Breadcrumbs Test', function (hooks) { setupApplicationTest(hooks); setupMirage(hooks); hooks.beforeEach(async function () { await setupRequiredEndpoints(this.server); // Clear breadcrumbs this.window = this.owner.lookup('service:browser/window'); this.window.localStorage.removeItem('lastBreadcrumbsState'); // Register common routes const commonRoutes = [ { routeName: 'tr-a-root/index', routeClass: createBCRoute(), controllerClass: createBCRouteCtrller( ALL_ROUTE_CRUMB_PROPS['tr-a-root'] ), }, // ... other common routes ]; commonRoutes.forEach((route) => setupRoute(this.owner, route)); }); // Test cases... });app/components/ak-date-picker/index.stories.js (1)
21-31: Add JSDoc comments to clarify expected data shapesConsider adding documentation to clarify the expected shape of the input parameters:
+/** + * @param {Object} collection - The collection object containing selected dates + * @param {Date} collection.date - The selected date from the collection + */ function onSelectCollection(collection) { this.set('selected', collection.date); } +/** + * @param {Object} range - The range object containing start and end dates + * @param {Date} range.date - The selected date range + */ function onSelectRange(range) { this.set('selected', range.date); } +/** + * @param {Object|null} day - The day object containing the selected date + * @param {Date} day.date - The selected date + */ function onSelectDay(day) { this.set('selected', day?.date); }app/components/ak-tabs/index.stories.js (1)
10-12: Consider using modern class syntaxThe function could be improved using modern class syntax with an arrow function for automatic binding:
-function onTabClick(tabId) { - this.set('activeTab', tabId); -} +onTabClick = (tabId) => { + this.activeTab = tabId; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(1 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(1 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (27)
- package.json
- app/components/ak-autocomplete/index.hbs
- app/components/file-compare/header/index.hbs
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/upload-app/status/details/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/organization-analytics/app-scan-chart/index.hbs
- tests/integration/components/partner/client-info/component-test.js
- .template-lintrc.js
- app/components/file-chart/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/security/project-search-list/index.hbs
- tests/integration/components/plus-n-list-test.js
- app/components/ak-checkbox/index.stories.js
- app/components/organization-team/overview/index.hbs
- app/components/ak-loader/index.hbs
- app/components/ak-modal/index.stories.js
- app/components/ak-autocomplete/index.stories.js
- app/components/security/analysis-list/index.hbs
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- app/components/ak-menu/index.stories.js
- app/components/ak-text-field/index.stories.js
- app/components/ak-pagination/index.stories.js
- app/components/ak-tree/index.stories.js
- app/components/security/file-search-list/index.hbs
- app/components/ak-drawer/index.stories.js
- app/components/project-settings/header/index.hbs
🔇 Additional comments (10)
app/components/ak-checkbox-tree/index.stories.js (3)
116-120: LGTM! Context setup looks good
The context setup properly uses the action helper to bind the handlers and maintains component state through args spreading.
112-113: LGTM! Template bindings are correct
The template bindings correctly use the @ prefix for named arguments and properly reference the handlers.
97-103: Verify consistent event handling patterns
Let's ensure this event handling pattern is consistently applied across other story files.
Also applies to: 116-120
✅ Verification successful
This broader search should help us:
- First identify all story files
- Look for event handler function patterns
- Look for
.setusage patterns
to better understand the consistency of event handling across story files.
Event handling pattern is consistent across story files
The event handling pattern in ak-checkbox-tree follows the established convention used throughout the component library's story files. The search results show consistent patterns where:
- Event handler functions follow the
onEventNamenaming convention - They use
this.set()to update component state - Similar patterns are found in other tree-like components (ak-tree) and various other components
Notable examples that follow the same pattern:
ak-tree: identicalonExpand/onCheckhandlersak-tabs:onTabClickwiththis.set('activeTab')ak-autocomplete:onChangewiththis.set('searchQuery')ak-modal:onClosewiththis.set('showModal')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in other story files
echo "Checking other story files for event handling patterns..."
rg -t js "function.*\(.*\).*\{\s*this\.set\(" app/components/
Length of output: 188
Script:
#!/bin/bash
# Let's try a broader search for story files and their event handling patterns
echo "Searching for story files..."
fd -e stories.js . app/components/
echo -e "\nChecking for event handling patterns in story files..."
rg -t js "function\s+on\w+\s*\([^)]*\)\s*\{" app/components/ --glob "*.stories.js" -A 2
echo -e "\nChecking for 'set' usage in story files..."
rg -t js "\.set\(" app/components/ --glob "*.stories.js" -A 1
Length of output: 7685
app/components/ak-popover/index.stories.js (2)
13-14: Add newline between functions for better readability
32-32: LGTM! Modern event handling approach
Good use of the {{on}} modifier and proper event handler binding. The approach aligns with modern Ember.js practices.
Also applies to: 44-46
app/components/ak-select/index.stories.js (1)
4-6: LGTM! Clean action handling refactor
The refactoring follows best practices:
- Function defined at module scope
- Proper action binding in args
- Consistent usage across templates
Also applies to: 23-23, 37-37, 63-63
app/components/ak-date-picker/index.stories.js (1)
86-86: LGTM! Consistent action binding across stories
The implementation properly handles different selection modes with appropriate action binding in each story variant.
Also applies to: 99-99, 110-110
app/components/ak-tabs/index.stories.js (2)
56-56: LGTM! Modern action binding syntax
The template correctly uses modern action binding syntax without the action helper.
108-108: LGTM! Consistent action binding
The CustomBadgeContent template maintains consistency with the modern action binding syntax.
app/components/ak-accordion/index.stories.js (1)
52-52: LGTM! Consistent modern action binding syntax
All templates have been correctly updated to use modern action binding syntax without the action helper.
Also applies to: 111-111, 126-126, 141-141, 176-176, 208-208, 235-235, 327-327, 348-348
Irene
|
||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1612-fix-ember-template-lint-errors
|
| Run status |
|
| Run duration | 05m 45s |
| Commit |
|
| Committer | Avi Shah |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
1
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/dynamic-scan.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| Dynamic Scan > it tests dynamic scan for an apk file: 58062 |
Test Replay
Screenshots
|
|
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
2 times, most recently
from
5f06ddc to
3b401c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/components/ak-date-picker/index.stories.js (1)
1-1: LGTM! Consider consolidating the handler functions.The handlers are well-implemented with proper null safety. However, they share similar patterns and could potentially be consolidated into a single handler.
Consider this alternative implementation to reduce code duplication:
-function onSelectCollection(collection) { - this.set('selected', collection.date); -} - -function onSelectRange(range) { - this.set('selected', range.date); -} - -function onSelectDay(day) { - this.set('selected', day?.date); -} +function onSelect(value) { + this.set('selected', value?.date); +}Also applies to: 21-31
app/components/ak-popover/index.stories.js (2)
10-16: Consider modernizing state managementInstead of using the classic
this.set(), consider using tracked properties or other modern Ember patterns for state management.-function handleMoreClick(event) { - this.set('anchorRef', event.currentTarget); -} +function handleMoreClick(event) { + this.anchorRef = event.currentTarget; +} -function handleClose() { - this.set('anchorRef', null); -} +function handleClose() { + this.anchorRef = null; +}
44-46: Consider consolidating redundant close handlersBoth
@onBackdropClickand@closeHandlerare set to the samehandleClosefunction. Consider consolidating these if they serve the same purpose to reduce complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(2 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(1 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (31)
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/security/analysis-list/index.hbs
- app/components/file-chart/index.hbs
- app/components/security/file-search-list/index.hbs
- app/components/security/project-search-list/index.hbs
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/organization-team/overview/index.hbs
- app/components/file-compare/header/index.hbs
- app/components/upload-app/status/details/index.hbs
- app/components/project-settings/header/index.hbs
- package.json
- app/components/ak-loader/index.hbs
- tests/integration/components/plus-n-list-test.js
- .template-lintrc.js
- app/components/ak-autocomplete/index.hbs
- app/components/ak-select/index.stories.js
- tests/integration/components/partner/client-info/component-test.js
- app/components/ak-autocomplete/index.stories.js
- app/components/ak-checkbox/index.stories.js
- app/components/ak-text-field/index.stories.js
- app/components/ak-tree/index.stories.js
- app/components/ak-checkbox-tree/index.stories.js
- app/components/ak-menu/index.stories.js
- app/components/ak-tabs/index.stories.js
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- app/components/ak-drawer/index.stories.js
- app/components/ak-modal/index.stories.js
- app/components/ak-pagination/index.stories.js
- app/components/ak-accordion/index.stories.js
🔇 Additional comments (8)
tests/acceptance/breadcrumbs-test.js (2)
Line range hint 1-1000: Test suite looks comprehensive and well-structured
The test suite effectively covers various breadcrumb navigation scenarios:
- Forward navigation through routes
- Backward navigation and breadcrumb cleanup
- Root route navigation and reset
- Route group switching
- Model-dependent navigation and recalculation
The assertions are thorough and the test setup is well-organized with helper functions and clear route configurations.
865-865: Update model binding syntax for consistency
The change from @model={{array 2}} to @model="1" introduces inconsistency in how models are passed to routes. Some routes use string literals while others use the array helper.
Let's verify the model binding patterns across the codebase:
✅ Verification successful
Based on the search results, I can now provide a final response:
Model binding syntax is consistent with codebase patterns
The change from array helper to string literal for model binding (@model="1" vs @models={{array 2}}) is actually correct. The codebase shows a clear pattern:
@modelis used for single values (strings, numbers)@modelsis used with the array helper for multiple values
Looking at the test file itself:
- Line 865:
@model="1"for single model parameter - Other usages:
@models={{array 2}}when passing multiple models
This change aligns with the established pattern in the codebase where single values use @model with direct values, while collections use @models with the array helper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check model binding patterns in templates
# Look for both string literals and array helper usage in model bindings
# Search for model bindings using string literals
echo "Model bindings using string literals:"
rg "@model=\"[^\"]+\"" -g "*.hbs"
# Search for model bindings using array helper
echo "\nModel bindings using array helper:"
rg "@model={{array" -g "*.hbs"
Length of output: 338
Script:
#!/bin/bash
# Let's modify our search to look for model bindings in both test files and templates
# First, let's look for model bindings in test files
echo "Model bindings in test files:"
rg "@model" -A 1 -B 1 tests/
# Then, let's look for model bindings in template files
echo -e "\nModel bindings in template files:"
fd -e hbs -x cat {} | grep -A 1 -B 1 "@model"
# Let's also check for AkLink component usage patterns
echo -e "\nAkLink component usage patterns:"
rg "<AkLink" -A 2 -B 2
Length of output: 57980
app/components/ak-date-picker/index.stories.js (3)
51-51: LGTM! Clean template binding.
The @onSelect binding follows modern Ember conventions and is properly formatted.
86-86: LGTM! Proper action binding in story exports.
The story exports correctly use the action helper to bind the appropriate handlers for each use case (single day, range, and collection selection).
Also applies to: 99-99, 110-110
Line range hint 1-110: Verify consistent implementation across other story files.
Let's ensure this pattern of using action helper is being applied consistently across other story files.
✅ Verification successful
No inconsistencies found in action helper usage for onSelect
The verification shows that:
- In
ak-date-picker/index.stories.js, all onSelect handlers consistently use theactionhelper:onSelect: action(onSelectDay),onSelect: action(onSelectRange), andonSelect: action(onSelectCollection) - Other occurrences of
onSelectin the codebase are either:- Test files with arrow functions (which is the correct pattern for tests)
- TypeScript files using different patterns appropriate for their context
- No template-lint issues were found related to the action helper usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other story files that might need similar updates
echo "Searching for story files with potential inconsistent action usage..."
rg -l "\.stories\.js$" | xargs rg "onSelect:[^a]"
echo "Checking for remaining template-lint issues..."
rg "ember-template-lint" . -A 5
Length of output: 9391
app/components/ak-popover/index.stories.js (3)
14-14: Add a newline above the handleClose function
Maintain consistent spacing between function declarations.
53-53: LGTM! Well-structured Storybook implementation
The action bindings and context setup follow Storybook best practices. The use of action() for method binding is correct.
Also applies to: 67-68
Line range hint 32-50: Verify template-lint compliance
Since this PR aims to fix ember-template-lint issues, let's verify that the template follows all current linting rules.
✅ Verification successful
Template follows modern linting practices
Based on the extensive codebase analysis, the template in ak-popover/index.stories.js follows modern template linting practices:
- Uses the recommended
{{on}}modifier for event handling, which is consistent with other components in the codebase as seen in the search results - Follows the established pattern for component attribute binding
- Matches the template-linting configuration in
.template-lintrc.jswhich extends the recommended ruleset - Aligns with the usage patterns found across multiple test and component files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for template-linting configuration and rules
cat .template-lintrc.js
# Search for similar patterns in other components to ensure consistent usage of {{on}} modifier
ast-grep --pattern 'AkIconButton @variant="outlined" {{on "click" $_}}'
Length of output: 400
Script:
#!/bin/bash
# Let's check for template-lint errors in the specific file
npx ember-template-lint app/components/ak-popover/index.stories.js
# Let's also check for any other similar patterns of {{on}} modifier usage
rg "{{on\s+'click'" -A 2 -B 2
# Check if there are any specific rules for {{on}} modifier in the codebase
rg "no-action|modifier-name-case|on-modifier" .template-lintrc.js
Length of output: 358
Script:
#!/bin/bash
# Let's try a different approach to check {{on}} modifier usage
rg "{{on 'click'" --type-add 'hbs:*.{hbs,js}' -t hbs
# Check for other template files in the same directory for consistency
fd . app/components/ak-popover/ -e hbs -e js
# Look for any template-lint related configurations
fd template-lint -e json -e js
Length of output: 307
Script:
#!/bin/bash
# Let's examine the actual template content
cat app/components/ak-popover/index.hbs
# Try a different pattern search with escaped braces
rg "\{\{on" --type-add 'hbs:*.{hbs,js}' -t hbs -A 2
# Check if there are any ember-template-lint errors in the package.json
cat package.json | grep -A 5 -B 5 "template-lint"
Length of output: 73634
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
3b401c4 to
87588de
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/components/ak-date-picker/index.stories.js (1)
102-102: 🛠️ Refactor suggestionRemove usage of
action()helper in context objects.While the templates have been updated to use modern action binding syntax, the context objects still use the deprecated
action()helper.Consider using arrow functions or class methods directly:
context: { ...args, - handleChange: action(handleChange), + handleChange() { + this.accordionExpanded = !this.accordionExpanded; + }, }Also applies to: 160-160, 259-259, 373-373
app/components/ak-accordion/index.stories.js (1)
102-102:⚠️ Potential issueRemove remaining
action()helper usage in context objectsWhile the templates have been modernized, the context objects still use the deprecated
action()helper. This is inconsistent with the PR's objective to upgrade ember-template-lint issues.Apply this change to all context objects:
context: { ...args, - handleChange: action(handleChange), + handleChange() { + this.accordionExpanded = !this.accordionExpanded; + }, }Also applies to: 160-160, 259-259, 373-373
🧹 Nitpick comments (5)
app/components/ak-date-picker/index.stories.js (1)
86-86: Consider using modern function binding syntax.While using the
actionhelper works, modern Ember encourages using class methods or arrow functions directly for better maintainability and cleaner syntax.- onSelect: action(onSelectDay), + onSelect: onSelectDay,Similarly for
onSelectRangeandonSelectCollection.Also applies to: 99-99, 110-110
app/components/ak-checkbox-tree/index.stories.js (2)
112-113: Document the event binding flowThe template references
this.onCheckandthis.onExpand, but these are actually standalone functions bound in the context. Consider adding a comment to clarify this relationship for better maintainability.+ {{! These handlers are standalone functions bound in the Template's context }} @onCheck={{this.onCheck}} @onExpand={{this.onExpand}}
116-120: Consider using modern Ember patterns for action handlingThe current implementation uses the
actionhelper to bind standalone functions. Consider refactoring to use a class-based approach with native class methods:-const Template = (args) => ({ +class Template extends Component { + @tracked expanded; + @tracked checked; + + constructor() { + super(...arguments); + Object.assign(this, args); + } + + @action + onExpand(expandedItems) { + this.expanded = expandedItems; + } + + @action + onCheck(checkedItems) { + this.checked = checkedItems; + } +}This approach:
- Uses tracked properties for state management
- Provides better type safety and autocomplete
- Makes the component's API more explicit
- Follows modern Ember patterns
tests/acceptance/breadcrumbs-test.js (2)
864-868: Consider adding tests for edge cases.The test suite would benefit from additional coverage for the following scenarios:
- Navigation with invalid model IDs
- Navigation with non-numeric model IDs
- Error handling for missing routes
Example test case structure:
test('It handles invalid model IDs gracefully', async function (assert) { await visit('/tr-b-root'); // Test invalid model ID await visit('/tr-b-root/parent-with-model/invalid-id'); assert.strictEqual(currentURL(), '/tr-b-root', 'Redirects to root on invalid model'); // Test non-numeric model ID await visit('/tr-b-root/parent-with-model/abc'); assert.strictEqual(currentURL(), '/tr-b-root', 'Redirects to root on non-numeric model'); });
Line range hint
864-1037: Enhance assertion messages for better test debugging.The test assertions would benefit from more descriptive messages that explain the expected state and actual outcome. This would make it easier to understand test failures.
Example improvements:
- doBreadcrumbItemsCompare( - akBreadcrumbsService.breadcrumbItems, - expectedCrumbs, - assert - ); + doBreadcrumbItemsCompare( + akBreadcrumbsService.breadcrumbItems, + expectedCrumbs, + assert, + 'Breadcrumb items should match expected crumbs after navigating to model with ID 1' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(2 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(2 hunks)app/components/side-nav/product-switcher/index.scss(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(1 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (30)
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- package.json
- app/components/file-compare/header/index.hbs
- app/components/organization-team/overview/index.hbs
- app/components/security/project-search-list/index.hbs
- app/components/security/file-search-list/index.hbs
- app/components/organization-analytics/app-scan-chart/index.hbs
- tests/integration/components/plus-n-list-test.js
- app/components/security/analysis-list/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/file-chart/index.hbs
- app/components/ak-select/index.stories.js
- app/components/upload-app/status/details/index.hbs
- app/components/project-settings/header/index.hbs
- app/components/ak-loader/index.hbs
- app/components/ak-popover/index.stories.js
- app/components/ak-autocomplete/index.hbs
- .template-lintrc.js
- app/components/ak-modal/index.stories.js
- tests/integration/components/partner/client-info/component-test.js
- app/components/side-nav/product-switcher/index.hbs
- app/components/ak-checkbox/index.stories.js
- app/components/ak-autocomplete/index.stories.js
- app/components/ak-text-field/index.stories.js
- app/components/ak-menu/index.stories.js
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- app/components/ak-tabs/index.stories.js
- app/components/ak-tree/index.stories.js
- app/components/ak-pagination/index.stories.js
- app/components/ak-drawer/index.stories.js
🔇 Additional comments (7)
app/components/side-nav/product-switcher/index.scss (1)
5-5: LGTM! The padding addition looks appropriate.
The added padding provides consistent spacing around the switcher component while maintaining the existing flex layout.
app/components/ak-date-picker/index.stories.js (3)
21-31: LGTM! Functions are well-structured and handle different selection types appropriately.
The implementation correctly updates the 'selected' property based on the selection type, with proper null handling in onSelectDay.
Line range hint 36-42: LGTM! Toggle functions are simple and effective.
The functions correctly implement the toggle functionality for both single and id-based state changes.
Line range hint 59-59: LGTM! Consistent usage of style helper across templates.
The style helper is properly used for applying inline styles, improving template consistency.
Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
app/components/ak-checkbox-tree/index.stories.js (1)
97-103: Modernize state management patterns
The event handlers are still using classic Ember patterns with this.set(). This issue was previously identified and the suggestion to use tracked properties remains valid.
tests/acceptance/breadcrumbs-test.js (1)
Line range hint 1-1037: Well-structured test file with good organization.
The test file demonstrates good practices:
- Clear separation of concerns
- Reusable helper functions and route configurations
- Logical progression of test cases
- Comprehensive coverage of navigation scenarios
app/components/ak-accordion/index.stories.js (1)
52-52: LGTM! Modern syntax updates look good
The changes to action bindings and style helpers are consistent with modern Ember practices:
- Removed
actionhelper in favor of direct method references - Consistently used
{{style}}helper for inline styles
Also applies to: 59-59, 81-81, 111-111, 117-117, 126-126, 132-132, 141-141, 148-148, 176-176, 192-192, 208-208, 220-220, 235-235, 246-246, 281-281, 303-303, 327-327, 335-335, 348-348, 356-356
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
87588de to
c61ab87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/components/ak-accordion/index.stories.js (1)
59-59: Consider extracting styles to CSS classesWhile the
{{style}}helper is correctly used, having these styles repeated across multiple instances suggests an opportunity for better maintainability:
- Extract common styles to CSS classes
- Use CSS-in-JS solutions or CSS modules if available in your setup
- Consider creating styled variants that encapsulate these common styles
This would improve maintainability and ensure consistent styling across all accordion instances.
Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(2 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(2 hunks)app/components/side-nav/product-switcher/index.scss(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(2 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (34)
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/security/project-search-list/index.hbs
- app/components/file-chart/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/security/analysis-list/index.hbs
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/project-settings/header/index.hbs
- app/components/side-nav/product-switcher/index.hbs
- .template-lintrc.js
- app/components/organization-team/overview/index.hbs
- app/components/file-compare/header/index.hbs
- tests/integration/components/plus-n-list-test.js
- app/components/security/file-search-list/index.hbs
- package.json
- app/components/ak-checkbox/index.stories.js
- app/components/upload-app/status/details/index.hbs
- app/components/ak-autocomplete/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/ak-text-field/index.stories.js
- app/components/ak-menu/index.stories.js
- app/components/ak-loader/index.hbs
- app/components/ak-select/index.stories.js
- app/components/ak-popover/index.stories.js
- app/components/ak-tree/index.stories.js
- app/components/ak-tabs/index.stories.js
- tests/integration/components/partner/client-info/component-test.js
- app/components/ak-modal/index.stories.js
- app/components/ak-autocomplete/index.stories.js
- app/components/ak-checkbox-tree/index.stories.js
- app/components/ak-drawer/index.stories.js
- app/components/ak-pagination/index.stories.js
- tests/acceptance/breadcrumbs-test.js
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- app/components/ak-date-picker/index.stories.js
🔇 Additional comments (4)
app/components/ak-accordion/index.stories.js (4)
36-42: Modernize state management using tracked properties
This implementation uses the older set() pattern which has been superseded by tracked properties in modern Ember.
52-52: LGTM! Action binding updates align with modern practices
The changes to use direct method references (e.g., @onChange={{this.handleChange}}) instead of the action helper follow current Ember best practices.
Also applies to: 111-111, 126-126, 141-141, 176-176, 208-208, 235-235, 327-327, 348-348
59-59: LGTM! Style updates use recommended helper
The changes to use the {{style}} helper for inline styles (e.g., {{style color='#ffffff' backgroundColor='#424651'}}) follow the recommended approach in Ember templates.
Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
102-102: Remove usage of action() helper in context objects
While the templates have been updated to use modern action binding syntax, the context objects still use the deprecated action() helper.
Also applies to: 160-160, 259-259, 373-373
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
c61ab87 to
dc3bd1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/acceptance/breadcrumbs-test.js (1)
865-868:⚠️ Potential issueInconsistent model parameter syntax.
The model parameter syntax
@model="1"is inconsistent with other model parameter usages in the file where the array helper is used (e.g.,@models={{array 2}}). This inconsistency could lead to confusion and potential issues with model handling.Apply this diff to maintain consistency:
- <AkLink @route="authenticated.tr-b-root.parent-with-model" @model="1" data-test-linkToRootBParentWithModel1> + <AkLink @route="authenticated.tr-b-root.parent-with-model" @models={{array "1"}} data-test-linkToRootBParentWithModel1>app/components/ak-accordion/index.stories.js (2)
36-42: 🛠️ Refactor suggestionModernize state management using tracked properties
The functions are using the older
set()pattern. Consider using Ember's modern tracked properties:-function handleChange() { - this.set('accordionExpanded', !this.accordionExpanded); -} - -function handleChangeId(id) { - this.set(id, !this[id]); -} +class AccordionStory { + @tracked accordionExpanded = false; + @tracked withIcon1 = false; + @tracked withIcon2 = false; + @tracked withIcon3 = false; + + handleChange = () => { + this.accordionExpanded = !this.accordionExpanded; + }; + + handleChangeId = (id) => { + this[id] = !this[id]; + }; +}
102-102: 🛠️ Refactor suggestionRemove usage of
action()helper in context objectsThe context objects still use the deprecated
action()helper. Consider using class methods directly:context: { ...args, - handleChange: action(handleChange), + handleChange() { + this.accordionExpanded = !this.accordionExpanded; + }, }Also applies to: 160-160, 259-259, 373-373
🧹 Nitpick comments (1)
app/components/ak-menu/index.stories.js (1)
33-39: Maintain consistency in event handling approachWhile you've updated
@onCloseand@onClickto use direct method references, the click handler still uses theonmodifier withthis. For consistency, consider updating all event handlers to use the same style:-<AkIconButton @variant="outlined" {{on 'click' this.handleMoreClick}}> +<AkIconButton @variant="outlined" @onClick={{this.handleMoreClick}}>Also applies to: 58-64, 87-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(2 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(2 hunks)app/components/side-nav/product-switcher/index.scss(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(2 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (32)
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/organization-team/overview/index.hbs
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/side-nav/product-switcher/index.scss
- app/components/security/file-search-list/index.hbs
- app/components/security/project-search-list/index.hbs
- app/components/file-chart/index.hbs
- app/components/security/analysis-list/index.hbs
- app/components/ak-autocomplete/index.ts
- app/components/file-compare/header/index.hbs
- app/components/ak-checkbox/index.stories.js
- app/components/project-settings/header/index.hbs
- .template-lintrc.js
- app/components/side-nav/product-switcher/index.hbs
- tests/integration/components/plus-n-list-test.js
- app/components/ak-select/index.stories.js
- app/components/upload-app/status/details/index.hbs
- app/components/ak-popover/index.stories.js
- app/components/ak-loader/index.hbs
- tests/integration/components/partner/client-info/component-test.js
- app/components/ak-autocomplete/index.hbs
- app/components/ak-modal/index.stories.js
- package.json
- app/components/ak-autocomplete/index.stories.js
- app/components/ak-checkbox-tree/index.stories.js
- app/components/ak-tree/index.stories.js
- app/components/ak-tabs/index.stories.js
- app/components/ak-date-picker/index.stories.js
- app/components/ak-text-field/index.stories.js
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- app/components/ak-pagination/index.stories.js
- app/components/ak-drawer/index.stories.js
🔇 Additional comments (3)
tests/acceptance/breadcrumbs-test.js (1)
Line range hint 1-1066: Well-structured and comprehensive test implementation!
The test suite demonstrates excellent practices:
- Clear test organization and descriptive names
- Proper setup and teardown
- Comprehensive coverage of breadcrumb scenarios
- Good error handling and assertions
app/components/ak-menu/index.stories.js (1)
53-53: LGTM: Common args spreading
The spreading of commonActionArgs across all story variants ensures consistent behavior.
Also applies to: 82-82, 120-120
app/components/ak-accordion/index.stories.js (1)
59-59: LGTM: Style helper usage
The update to use the {{style}} helper for inline styles is a good improvement, making the styles more maintainable and consistent with Ember's template syntax.
Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
dc3bd1c to
47eba4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/components/ak-menu/index.stories.js (1)
10-20: 🛠️ Refactor suggestionModernize state management using tracked properties
The current implementation uses the older
set()pattern and action helper. Consider using modern tracked properties as suggested in previous reviews.
🧹 Nitpick comments (4)
app/components/side-nav/product-switcher/index.hbs (1)
17-21: Consider extracting duplicated icon code into a named block.The icon implementation is duplicated between expanded and collapsed states. Consider extracting it into a named block for better maintainability.
+ {{#let (component 'ak-icon' + data-test-side-menu-switcher-icon=true + [email protected] + iconName='apps' + ) as |SwitcherIcon|}} {{#if this.isSidebarExpanded}} <AkButton data-test-side-menu-switcher local-class='switcher expanded' {{on 'click' this.onClickSwitcher}} > - <AkIcon - data-test-side-menu-switcher-icon - class={{@classes.menuItemIcon}} - @iconName='apps' - /> + <SwitcherIcon /> ... </AkButton> {{else}} <AkIconButton data-test-side-menu-switcher local-class='switcher collapsed' {{on 'click' this.onClickSwitcher}} > - <AkIcon - data-test-side-menu-switcher-icon - class={{@classes.menuItemIcon}} - @iconName='apps' - /> + <SwitcherIcon /> </AkIconButton> {{/if}} + {{/let}}Also applies to: 48-52
app/components/ak-menu/index.stories.js (2)
33-39: DRY: Extract common template patternThe same menu button and event handling pattern is duplicated across three templates. Consider extracting this into a shared template:
const MenuButtonTemplate = hbs` <AkIconButton @variant="outlined" {{on 'click' this.handleMoreClick}}> <AkIcon @iconName="more-vert" /> </AkIconButton> <AkMenu @arrow={{this.arrow}} @anchorRef={{this.anchorRef}} @onClose={{this.handleClose}} as |akm|> {{yield akm}} </AkMenu> `; // Usage in templates: const Template = (args) => ({ template: hbs` {{#let (component MenuButtonTemplate) as |MenuButton|}} <MenuButton as |akm|> {{#each this.menuData as |data|}} <akm.listItem @onClick={{this.handleClose}} @divider={{data.divider}}> {{yield akm data}} </akm.listItem> {{/each}} </MenuButton> {{/let}} `, context: { ...args, menuData }, });Also applies to: 58-64, 87-92
45-45: Simplify context managementThe same context spreading pattern is duplicated across templates. Consider creating a base context factory:
const createMenuContext = (args) => new MenuStory({ ...args, menuData }); const Template = (args) => ({ template: hbs`...`, context: createMenuContext(args), });Also applies to: 74-74, 111-111
app/components/ak-accordion/index.stories.js (1)
59-59: Consider extracting repeated styles to a CSS classThe same inline styles are repeated across multiple content blocks. Consider extracting these to a CSS class for better maintainability:
+/* app/styles/components/ak-accordion.css */ +.accordion-content { + padding: 0.75rem; + margin: 0.5rem; + color: #ffffff; + background-color: #424651; +} -<div class="p-3 m-2" {{style color='#ffffff' backgroundColor='#424651'}} > +<div class="accordion-content">Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(2 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(2 hunks)app/components/side-nav/product-switcher/index.scss(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(2 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (30)
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/ak-autocomplete/index.ts
- app/components/organization-team/overview/index.hbs
- app/components/security/file-search-list/index.hbs
- tests/integration/components/partner/client-info/component-test.js
- app/components/file-compare/header/index.hbs
- app/components/security/project-search-list/index.hbs
- app/components/ak-loader/index.hbs
- .template-lintrc.js
- tests/integration/components/plus-n-list-test.js
- app/components/upload-app/status/details/index.hbs
- app/components/file-chart/index.hbs
- package.json
- app/components/ak-autocomplete/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/ak-text-field/index.stories.js
- app/components/project-settings/header/index.hbs
- app/components/ak-modal/index.stories.js
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/security/analysis-list/index.hbs
- app/components/ak-checkbox/index.stories.js
- app/components/ak-checkbox-tree/index.stories.js
- tests/acceptance/breadcrumbs-test.js
- app/components/ak-select/index.stories.js
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
- app/components/ak-drawer/index.stories.js
- app/components/ak-tabs/index.stories.js
- app/components/ak-date-picker/index.stories.js
- app/components/ak-popover/index.stories.js
- app/components/ak-pagination/index.stories.js
🔇 Additional comments (4)
app/components/side-nav/product-switcher/index.hbs (1)
12-16: LGTM! Improved accessibility with semantic buttons.
The change from a custom element with role='button' to semantic button elements (AkButton/AkIconButton) is an improvement as it:
- Uses semantic HTML elements (preferred over ARIA roles)
- Provides built-in keyboard accessibility
- Maintains consistent behavior
Note: The previous review comment about keeping role='button' is no longer relevant as we're now using proper button elements.
Also applies to: 43-47
app/components/ak-accordion/index.stories.js (3)
36-42: Consider using tracked properties for state management
The new functions use the older set() pattern. As previously suggested, consider using tracked properties for modern state management.
102-102: Remove usage of action() helper in context objects
The context objects still use the deprecated action() helper. As previously suggested, consider using arrow functions directly.
Also applies to: 160-160, 259-259, 373-373
52-52: Verify accessibility attributes for accordion interactions
Ensure that the accordion component properly handles accessibility attributes (aria-expanded, aria-controls, etc.) for screen readers.
Run the following script to check for accessibility attributes in the component implementation:
Also applies to: 111-111, 126-126, 141-141, 176-176, 208-208, 235-235, 327-327, 348-348
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
47eba4d to
c27dc6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/components/ak-tree/index.stories.js (1)
83-89: 🛠️ Refactor suggestionModernize state management approach
The current implementation uses older Ember patterns with
this.set()and module-scope functions. A previous review already suggested a more modern, class-based approach which would be more maintainable and align better with current Ember best practices.app/components/ak-autocomplete/index.stories.js (1)
54-54: 🛠️ Refactor suggestionRemove usage of
action()helper in context objectsWhile the template has been updated to use modern action binding syntax, the context object still uses the deprecated
action()helper.context: { ...args, - handleChange: action(onChange), + handleChange(searchValue) { + this.searchQuery = searchValue; + }, options: args.objectOptions ? objectOptions : stringOptions, },app/components/ak-date-picker/index.stories.js (1)
86-86: 🛠️ Refactor suggestionRemove usage of
action()helper in context objectsThe story exports still use the deprecated
action()helper. Consider using class methods directly:Basic.args = { ...commonArgs, selected: new Date(), - onSelect: action(onSelectDay), + onSelect(day) { + this.selected = day?.date; + }, }; RangeSelection.args = { ...commonArgs, - onSelect: action(onSelectRange), + onSelect(range) { + this.selected = range.date; + }, }; MultipleSelection.args = { ...commonArgs, - onSelect: action(onSelectCollection), + onSelect(collection) { + this.selected = collection.date; + }, };Also applies to: 99-99, 110-110
app/components/ak-accordion/index.stories.js (2)
36-42: 🛠️ Refactor suggestionModernize state management using tracked properties
The functions are using the older
set()pattern. Consider refactoring to use modern Ember's tracked properties:-function handleChange() { - this.set('accordionExpanded', !this.accordionExpanded); -} - -function handleChangeId(id) { - this.set(id, !this[id]); -} +class AccordionStory { + @tracked accordionExpanded = false; + + handleChange = () => { + this.accordionExpanded = !this.accordionExpanded; + }; + + handleChangeId = (id) => { + this[id] = !this[id]; + }; +}
102-102: 🛠️ Refactor suggestionRemove usage of
action()helper in context objectsThe story contexts still use the deprecated
action()helper. Consider using class methods directly:context: { ...args, - handleChange: action(handleChange), + handleChange() { + this.accordionExpanded = !this.accordionExpanded; + }, }Also applies to: 160-160, 259-259, 373-373
🧹 Nitpick comments (2)
app/components/ak-tree/index.stories.js (2)
130-131: Consider using arrow functions or class methods for consistent context bindingThe current implementation mixes patterns by using the
actionhelper in story args while using modern template bindings. This could lead to context binding issues.Consider either:
- Using arrow functions to maintain context:
Default.args = { treeData: TREE_DATA, expanded: ['Root1', 'Root2', 'Root3'], checked: [], - onExpand: action(onExpand), - onCheck: action(onCheck), + onExpand: (expandedItems) => this.set('expanded', expandedItems), + onCheck: (checkedItems) => this.set('checked', checkedItems), };
- Or better yet, implement the class-based approach suggested in the previous review, which would provide a more consistent and maintainable solution.
Line range hint
103-109: Enhance accessibility for tree navigationThe tree structure could benefit from improved accessibility:
- Add appropriate ARIA attributes (
role="tree",role="treeitem")- Include keyboard navigation support
- Add aria-expanded state for expandable nodes
Example improvements:
{{#if node.isParent}} <AkIcon @iconName='arrow-drop-down' @color='secondary' + role="button" + aria-expanded={{node.expanded}} {{style cursor="pointer"}} {{on 'click' (fn tree.onExpand node)}} + {{on 'keydown' (fn tree.onKeyDown node)}} + tabindex="0" /> {{/if}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.template-lintrc.js(1 hunks)app/components/ak-accordion/index.stories.js(21 hunks)app/components/ak-autocomplete/index.hbs(1 hunks)app/components/ak-autocomplete/index.stories.js(3 hunks)app/components/ak-autocomplete/index.ts(1 hunks)app/components/ak-checkbox-tree/index.stories.js(3 hunks)app/components/ak-checkbox/index.stories.js(3 hunks)app/components/ak-date-picker/index.stories.js(6 hunks)app/components/ak-drawer/index.stories.js(2 hunks)app/components/ak-loader/index.hbs(2 hunks)app/components/ak-menu/index.stories.js(7 hunks)app/components/ak-modal/index.stories.js(5 hunks)app/components/ak-pagination/index.stories.js(6 hunks)app/components/ak-popover/index.stories.js(5 hunks)app/components/ak-select/index.stories.js(6 hunks)app/components/ak-tabs/index.stories.js(6 hunks)app/components/ak-text-field/index.stories.js(2 hunks)app/components/ak-tree/index.stories.js(3 hunks)app/components/file-chart/index.hbs(1 hunks)app/components/file-compare/header/index.hbs(1 hunks)app/components/organization-analytics/app-scan-chart/index.hbs(2 hunks)app/components/organization-team/overview/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.hbs(1 hunks)app/components/partner/credit-transfer/credit-transfer-input/index.scss(1 hunks)app/components/project-settings/header/index.hbs(1 hunks)app/components/security/analysis-list/index.hbs(1 hunks)app/components/security/file-search-list/index.hbs(1 hunks)app/components/security/project-search-list/index.hbs(1 hunks)app/components/side-nav/product-switcher/index.hbs(2 hunks)app/components/side-nav/product-switcher/index.scss(1 hunks)app/components/upload-app/status/details/index.hbs(1 hunks)package.json(2 hunks)tests/acceptance/breadcrumbs-test.js(1 hunks)tests/integration/components/ak-tabs/index-test.js(0 hunks)tests/integration/components/partner/client-info/component-test.js(1 hunks)tests/integration/components/plus-n-list-test.js(1 hunks)
💤 Files with no reviewable changes (1)
- tests/integration/components/ak-tabs/index-test.js
🚧 Files skipped from review as they are similar to previous changes (30)
- app/components/ak-autocomplete/index.ts
- app/components/security/analysis-list/index.hbs
- app/components/partner/credit-transfer/credit-transfer-input/index.scss
- app/components/ak-loader/index.hbs
- app/components/security/file-search-list/index.hbs
- app/components/ak-autocomplete/index.hbs
- app/components/upload-app/status/details/index.hbs
- .template-lintrc.js
- app/components/organization-analytics/app-scan-chart/index.hbs
- app/components/ak-checkbox/index.stories.js
- tests/integration/components/plus-n-list-test.js
- app/components/security/project-search-list/index.hbs
- app/components/file-chart/index.hbs
- app/components/organization-team/overview/index.hbs
- tests/integration/components/partner/client-info/component-test.js
- app/components/file-compare/header/index.hbs
- app/components/side-nav/product-switcher/index.scss
- app/components/ak-text-field/index.stories.js
- app/components/ak-select/index.stories.js
- app/components/ak-modal/index.stories.js
- app/components/ak-checkbox-tree/index.stories.js
- app/components/ak-popover/index.stories.js
- app/components/project-settings/header/index.hbs
- app/components/ak-menu/index.stories.js
- app/components/side-nav/product-switcher/index.hbs
- app/components/ak-pagination/index.stories.js
- app/components/ak-drawer/index.stories.js
- app/components/ak-tabs/index.stories.js
- package.json
- app/components/partner/credit-transfer/credit-transfer-input/index.hbs
🔇 Additional comments (4)
tests/acceptance/breadcrumbs-test.js (1)
865-865: Inconsistent model parameter syntax.
The model parameter syntax @model="1" is inconsistent with other model parameter usages in the file where the array helper is used (e.g., @models={{array 2}}). This inconsistency could lead to confusion and potential issues with model handling.
Apply this diff to maintain consistency:
- <AkLink @route="authenticated.tr-b-root.parent-with-model" @model="1" data-test-linkToRootBParentWithModel1>
+ <AkLink @route="authenticated.tr-b-root.parent-with-model" @models={{array "1"}} data-test-linkToRootBParentWithModel1>app/components/ak-tree/index.stories.js (1)
98-99: LGTM! Modern template binding syntax
The template bindings correctly use direct method references with the @ prefix convention for named arguments.
app/components/ak-autocomplete/index.stories.js (1)
28-30: 🛠️ Refactor suggestion
Modernize state management using tracked properties
The current implementation uses the older set() pattern. Consider using modern Ember's tracked properties:
-function onChange(searchValue) {
- this.set('searchQuery', searchValue);
-}
+class AutocompleteStory {
+ @tracked searchQuery;
+
+ handleChange = (searchValue) => {
+ this.searchQuery = searchValue;
+ };
+}Likely invalid or redundant comment.
app/components/ak-accordion/index.stories.js (1)
59-59: LGTM! Style updates look good
The updates to use the {{style}} helper for inline styles are consistent and follow best practices.
Also applies to: 81-81, 117-117, 132-132, 148-148, 192-192, 220-220, 246-246, 281-281, 303-303, 335-335, 356-356
avzz-19
force-pushed
the
PD-1612-fix-ember-template-lint-errors
branch
from
c27dc6c to
2b6703f
Compare
|
future-pirate-king
deleted the
PD-1612-fix-ember-template-lint-errors
branch
No description provided.