Conversation
🦋 Changeset detectedLatest commit: 7f7ca58 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds ChangesGeneric list components
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant IxListItem
participant IxList
participant Consumer
User->>IxListItem: activate item or selection control
IxListItem->>Consumer: emit itemClick or selectedChange
User->>IxListItem: start keyboard or pointer drag
IxListItem->>IxList: provide drag interaction
IxList->>IxList: update order and focus
IxList->>Consumer: emit itemOrderChange
Merge Risk: 🟡 Moderate · up to The new list component still has keyboard and accessibility defects, an inconsistent documented API, and failing component tests that can block users and leave required checks incomplete; it is not merge-ready until these issues are fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 33 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
| this.announce( | ||
| item, | ||
| `${this.getItemLabel( | ||
| item | ||
| )} lifted. Use arrow keys to move, Enter or Space to drop, and Escape to cancel.` | ||
| ); |
There was a problem hiding this comment.
Just put the comment here but hit every hardcoded announce string, we need to provide a way to make it i18n ready
| item.style.setProperty('--ix-list-drag-left', `${itemBounds.left}px`); | ||
| item.style.setProperty('--ix-list-drag-top', `${itemBounds.top}px`); | ||
| item.style.setProperty('--ix-list-drag-width', `${itemBounds.width}px`); |
There was a problem hiding this comment.
Would make sense to use here also rem
There was a problem hiding this comment.
i dont think rem makes sense here, because getBoundingClientRect() returns the values in px, and writing them back in px preserves the same system
danielleroux
left a comment
There was a problem hiding this comment.
Storybook for "ix-list-item" is missing
|



EIX-65
Docs PR: siemens/ix-docs#279
Add ix-list, ix-list-item and ix-list-separator.
Implements the following functionality:
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)Summary by CodeRabbit
New Features
Tests