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

Run jest only from the root #500

Merged
merged 4 commits into from Oct 19, 2018
Merged

Run jest only from the root #500

merged 4 commits into from Oct 19, 2018

Conversation

TrySound
Copy link
Contributor

This allows to reduce configuration and modules count per each package.

All dependencies (except rebass and styled-system) are reused from the
root. With yarn workspaces all depenendecies could be hoised without
need to remove them from each packages. That's why I removed evenr react
packages.

@TrySound
Copy link
Contributor Author

@jxnblk I already implemented packages builder locally and gonna send a few pull requests separately starting with this one.

@TrySound
Copy link
Contributor Author

Installing dependencies for each package sucks to be honest. And there are only two packages yet.

@TrySound
Copy link
Contributor Author

TrySound commented Sep 22, 2018

@jxnblk Is this looking fine?

expect(json).toHaveStyleRule('bottom', '0')
expect(json).toHaveStyleRule('left', '0')
})
test('Position renders', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to remove the describe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is cleaner considering that the file is already scope. describe helps with deeper scope like in root tests.

Copy link
Member

Choose a reason for hiding this comment

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

Sure but this change is unrelated to the PR

import React from 'react'
import TestRenderer from 'react-test-renderer'
import Hide from '../src'

const renderJSON = el => TestRenderer.create(el).toJSON()

describe('@rebass/hide', () => {

test.skip('Mapped renders', () => {
Copy link
Member

Choose a reason for hiding this comment

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

These tests weren't working before – I'd like to have the tests check for rules scoped by media query – even if they're not running they're intended as placeholders to fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is used not existing component and do not different from the next one.

Copy link
Member

Choose a reason for hiding this comment

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

This is still a placeholder that I want to keep, even if it's not up to date

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

@jxnblk
Copy link
Member

jxnblk commented Sep 22, 2018

Thanks! This looks a lot cleaner. Let me pull this down locally and give it a shot, but would like to get this in soonish

This allows to reduce configuration and modules count per each package.

All dependencies (except rebass and styled-system) are reused from the
root. With yarn workspaces all depenendecies could be hoised without
need to remove them from each packages. That's why I removed evenr react
packages.
@TrySound
Copy link
Contributor Author

Rebased

@TrySound
Copy link
Contributor Author

@jxnblk The next PR is ready locally. Will send after merging this.

expect(json).toMatchSnapshot()
expect(json).toHaveStyleRule('display', 'none', {
// media: 'screen and (min-width:40em)'
Copy link
Member

Choose a reason for hiding this comment

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

I think this was maybe a bug with jest-styled-components, would be good to have tests for the media query aspect of this component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug still appears. There is not media queries with any combination. Reverted comment. Not related to this PR.

@TrySound
Copy link
Contributor Author

@jxnblk Is this ok for you?

@TrySound
Copy link
Contributor Author

TrySound commented Oct 2, 2018

Friendly ping @jxnblk

@TrySound
Copy link
Contributor Author

TrySound commented Oct 7, 2018

Any news @jxnblk ?

@TrySound
Copy link
Contributor Author

Ping @jxnblk

@jxnblk
Copy link
Member

jxnblk commented Oct 19, 2018

Thanks for this! And sorry haven't done any work on Rebass lately

@jxnblk jxnblk merged commit 64551f5 into rebassjs:master Oct 19, 2018
@TrySound TrySound deleted the hoist-jest branch October 19, 2018 22:04
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