-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(tree): allow selection of parent category w/out selecting children #6926
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just had some feedback on filtering out specific elements from the onclick handler.
Running the e2e again as they somehow started failing? Is this normal? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ✨
@driskull or @jcfranco do you have any ideas on the e2e tests? Seeing it locally too. I went back to the commit that passed and it also failed, so wondering what could have changed?
hmmm, the actions runner reports a tooltip test failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖
↖🌳↖↖↖🌳↖🌳🌳🌳↖↖🌳🌳🌳↖🌳🌳🌳🌳↖🌳↖
↖🌳🌳↖↖🌳↖↖🌳↖↖🌳↖↖↖↖🌳↖↖↖↖🌳↖
↖🌳↖🌳↖🌳↖↖🌳↖↖🌳↖↖↖↖🌳🌳🌳↖↖🌳↖
↖🌳↖↖🌳🌳↖↖🌳↖↖🌳↖↖↖↖🌳↖↖↖↖↖↖
↖🌳↖↖↖🌳↖🌳🌳🌳↖↖🌳🌳🌳↖🌳🌳🌳🌳↖🌳↖
↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖↖
I'll take a look at the unstable test. 👀
src/components/tree/tree.tsx
Outdated
@@ -165,23 +165,26 @@ export class Tree { | |||
(target.hasChildren && | |||
(this.selectionMode === "children" || this.selectionMode === "multichildren"))); | |||
|
|||
// deselecting a parent in multichildren should deselect all the children |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment can be dropped as the line below is expressive enough.
Talked to @driskull and I think found a way to stabilize the tooltip tests. Feel free to skip them (https://github.com/Esri/calcite-components/blob/master/src/components/tooltip/tooltip.e2e.ts#L730 and https://github.com/Esri/calcite-components/blob/master/src/components/tooltip/tooltip.e2e.ts#L791) to get this installed and I'll submit a separate PR for restoring/stabilizing the skipped tests. |
I skipped the tests and addressed the pr comment, will see how the e2e tests go |
Thanks again, @paulcpederson! Can you open a follow-up issue for the pending styling/focus/hover/accessibility updates? |
Added a follow-up issue targeted for August regarding the hover states in #6930. @paulcpederson were there other considerations with styling, focus or a11y for another follow-up issue? Didn't see other context in Figma or the linked issues. |
Nope, the styling was left as is for now. I think your hover states issue should cover updating that. Accessibility has it's own issue IIRC. Nothing more to open for this, thanks for opening that issue. And thanks @jcfranco for the merge |
Related Issue: #6912, #6444, #6509
Summary
Updates the interaction model of trees to adhere to the new design. The key changes to the interaction model are:
Note: this pr does not address the hover states in that design. We are coming down to the wire and I want to give time for code review. I can help address the styling/focus/hover/accessibility aspects of the tree next release.