Skip to content

fix: Specify baseElement without container #394

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 24, 2019
Merged

fix: Specify baseElement without container #394

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2bf0cb5
Add expected behavior of base element
eps1lon Jun 24, 2019
f145b3a
Fix expected behavior of baseElement
eps1lon Jun 24, 2019
b962333
Improve tests
eps1lon Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/__tests__/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,44 @@ test('renders options.wrapper around node', () => {
</div>
`)
})

describe('baseElement', () => {
Copy link
Member

@kentcdodds kentcdodds Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really prefer to not use any nesting. This is all for just one test, please instead move the contents of beforeAll and afterAll to the inside of the test and use the test global rather than the describe and it globals.

Copy link
Member Author

@eps1lon eps1lon Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks two other test that assume an empty body. The nesting is for isolation. What is the problem with that? The alternative is to create a new test which is worse IMO.

Copy link
Member

@kentcdodds kentcdodds Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the direction that nesting puts us in. It's a slippery slope IMO. Maybe I'm just too scarred by past testbases. I'm much more willing to create a new test file than nesting tests.

Another thing we could do is just take the beforeAll and afterAll and put them inside the test. That would avoid the problems altogether. Could you do that please?

let baseElement
beforeAll(() => {
baseElement = document.createElement('div')
document.body.appendChild(baseElement)
})

afterAll(() => {
baseElement.parentNode.removeChild(baseElement)
})

it('can take a custom element for isolation', () => {
function DescribedButton({title: description, ...buttonProps}) {
Copy link
Member

@kentcdodds kentcdodds Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary that this component be so complex? I prefer to make the component simpler/contrived to get the point across. I believe that this test would pass with or without the baseElement... Can we have a test that actually requires the baseElement?

Copy link
Member Author

@eps1lon eps1lon Jun 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give it a try to make this more meaningful. It does, however, fail without the fix applied or without the baseElement passed.

return (
<React.Fragment>
<button {...buttonProps} aria-describedby="tooltip" />
{ReactDOM.createPortal(
<div id="tooltip" role="tooltip">
{description}
</div>,
document.body,
)}
</React.Fragment>
)
}

const {getByRole, getByText} = render(
<DescribedButton description="this descripton is hidden from rtl">
Click me
</DescribedButton>,
{baseElement},
)

expect(getByText('Click me')).toBeTruthy()
expect(document.querySelector('[role="tooltip"]')).toBeTruthy()
expect(() => getByRole('tooltip')).toThrow(
'Unable to find an element by [role=tooltip]',
)
})
})
6 changes: 4 additions & 2 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ function render(
wrapper: WrapperComponent,
} = {},
) {
if (!container) {
if (!baseElement) {
// default to document.body instead of documentElement to avoid output of potentially-large
// head elements (such as JSS style blocks) in debug output
baseElement = document.body
container = document.body.appendChild(document.createElement('div'))
}
if (!container) {
container = baseElement.appendChild(document.createElement('div'))
}

// we'll add it to the mounted containers regardless of whether it's actually
Expand Down