PD-12484 Cypress tests for Sortable component 🐐 #110
Conversation
| {children} | ||
| </div> | ||
| {onRemove && ( | ||
| <div css={itemCloseStyles} data-component-type="sortable.item.remove"> |
There was a problem hiding this comment.
Too much to add data-component-type throughout the DOM to allow precise control over styling (or testing)? Or maybe just a different attribute name would be more appropriate since these aren't really components?
There was a problem hiding this comment.
Hm... data-type makes me think of arrays and hashmaps... maybe data-element? Or even data-element-type to maintain a similar naming pattern?
There was a problem hiding this comment.
ohhh I suggested data-type because I thought data-component-type was too long, but on that note, I would suggested data-react-display-name="" maybe? or just data-display-name?
There was a problem hiding this comment.
Oh. Yeah, both component-type and display-name are used in the issue #101 so it's not clear what is actually being proposed.
I like component-type better, but since we already set the .displayName for our components as a convention (https://github.com/acl-services/paprika/blob/master/packages/Sortable/src/Sortable.js#L87) maybe it's best to keep with the same name to reduce cognitive load?
I was worried this might derail this PR, and it's not really that important at this point, but it's also a decision I would rather force now than just kick down the road.
There was a problem hiding this comment.
yes, let's keep it with data-display-name then.
packages/Sortable/src/Sortable.js
Outdated
| css={sortableStyles} | ||
| data-display-name="sortable" |
There was a problem hiding this comment.
In addition to #101 (comment), this is even more confusing to me because it's on a DOM component. To an outside observer, this looks like some magical way of setting displayName on this <ul>, which it isn't.
packages/Sortable/package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@paprika/sortable", | |||
| "version": "0.1.0", | |||
| "version": "0.1.1", | |||
There was a problem hiding this comment.
I don't think we want to update versions manually in PRs. It should be handled by lerna when you publish (at which point you determine whether it's a major/minor/patch bump). cc @nahumzs
There was a problem hiding this comment.
How about the devdependencies in paprika/package.json file? Do we need to manually update it to keep it updated? @alexzherdev @nahumzs
There was a problem hiding this comment.
Good question, I suspect it should be part of our publishing procedure though.
There was a problem hiding this comment.
Lerna allowed the option of manually bumping the version vs automatically handle. Maybe we should discuss this a little further, but right now you don't need to bump the version mike
There was a problem hiding this comment.
Yep, reverted in a more recent commit.
🛠 Purpose
Add test coverage for the new
<Sortable>component.✏️ Notes to Reviewer
Test cases:
✓ it should be re-orderable by mouse
✓ it should ignore out-of-bounds drops
✓ it should be re-orderable by keyboard
✓ it should allow items to be removed
Was unable to get tests working with
react-testing-library, despite one person's claim🌟 Bonus
morePropsto root-level<ul>element.provided.placeholderto<Droppable>so that its height is maintained while dragging.data-component-typedata-pka-anchorattributes all over as per issue RFC Adding an anchor attribute for each component. #101