Skip to content
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(responsive): use defaultmatches for all uses #471

Merged
merged 3 commits into from
Feb 8, 2018
Merged

Conversation

theetrain
Copy link
Contributor

  • test: add ssr tests for Box, Button, ExpandCollapse, DisplayHeading, Heading, Tooltip
  • upgrade jest to 22.x
  • add testid to ExpandCollapse tertiarytext in order to be selectable

Description

- test: add ssr tests for Box, Button, ExpandCollapse, DisplayHeading, Heading, Tooltip
- upgrade jest to 22.x
- add testid to ExpandCollapse tertiarytext in order to be selectable
@@ -83,7 +83,7 @@
"gitbook-cli": "^2.3.2",
"husky": "^0.14.3",
"identity-obj-proxy": "^3.0.0",
"jest": "^21.2.1",
"jest": "^22.2.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What an odd coincidence, the latest version of Jest happens to have the same minor and patch version integer. 🍨


import Box from '../Box'

describe('responsive behaviour', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this describe can be a bit more specific so that it will show up properly in the output. The behaviour being tested is also about SSR I believe, not responsiveness.

Perhaps it should be:

describe('Box server side rendering', ...)


import Button from '../Button'

describe('responsive behaviour', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as Box...


import ExpandCollapse from '../ExpandCollapse'

describe('responsive behaviour', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as Box...

@@ -4,6 +4,7 @@ import { mount, render } from 'enzyme'
import { warn } from '../../../../utils/warn'

import ButtonLink from '../ButtonLink'
import Responsive from '../../../Responsive/Responsive'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a "ButtonLink.ssr.spec.jsx" file..... I think this test should be rewritten to follow the SSR pattern established in the other components.


import Tooltip from '../Tooltip'

describe('responsive behaviour', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as Box...


import DisplayHeading from '../DisplayHeading'

describe('responsive behaviour', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as Box...


import Heading from '../Heading'

describe('responsive behaviour', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as Box...

it('renders', () => {
const heading = render(<Heading level="h2">The heading</Heading>)

expect(heading).toMatchSnapshot()
})

it('renders mobile styles by default', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think since we have a "Heading.ssr.spec.jsx" file, then this test is not necessary.

Copy link
Contributor

@ryanoglesby08 ryanoglesby08 left a comment

Choose a reason for hiding this comment

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

Looking pretty good! Seems like there are a couple of extra doShallows in there though...


return button
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this doShallow still used? Why was it added if its not used? If it is used, it is called doShallow but it does a mount..... something is awry here.....


return heading
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this doShallow still used? Why was it added?

@ryanoglesby08 ryanoglesby08 merged commit 314bc55 into master Feb 8, 2018
@ryanoglesby08 ryanoglesby08 deleted the fix/TDS-517 branch February 8, 2018 19:07
theetrain added a commit that referenced this pull request Feb 26, 2018
* fix(responsive): use defaultmatches for all uses

- test: add ssr tests for Box, Button, ExpandCollapse, DisplayHeading, Heading, Tooltip
- upgrade jest to 22.x
- add testid to ExpandCollapse tertiarytext in order to be selectable

* test: adjust server side rendering descriptors

* refactor(tests): remove dirty laundry 👕
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants