-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[TreeItem] allow conditional child #20154 #20156
Conversation
tonyhallett
commented
Mar 17, 2020
- I have followed (at least) the PR section of the contributing guide.
</TreeItem> | ||
</TreeView>, | ||
), | ||
).to.not.throw(); |
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.
It would be awesome if you could be more specific about the expected output. For example should nodeId="1"
be considered expanded or is it a leaf node?
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.
@tonyhallett Did you accidentally resolve this comment? Not sure how this was addressed.
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.
Have mirrored the test in the TreeView. Apologies for the addiitonal prettier commit
Details of bundle changes.Comparing: 7e1da61...854c0fd Details of page changes
|
); | ||
} | ||
const { getByText, queryByText } = render(<TestComponent />); | ||
expect(getByText('test')).to.not.be.null; |
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.
Please don't use getByText. It's extremely brittle. You can leverage ByTestId since you already added testids.
Also: what is the expected aria-expanded before and after the click?
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.
Ok, as I said I mirrored the existing TreeView test. There are many getByText.
Will update
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.
Ok, as I said I mirrored the existing TreeView test. There are many getByText.
No worries. Sometimes these slip by.
@eps1lon pls can you advise on the failure. |
@tonyhallett Flaky netlify deploy. You need to rebase anyway so this should resolve itself. |
Going to be easier to just do the change again |