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

feat: add factory for SpectatorForDirective #157

Merged
merged 1 commit into from
Aug 19, 2019
Merged

Conversation

dirkluijk
Copy link
Collaborator

@dirkluijk dirkluijk commented Aug 19, 2019

So what I basically did:

  • Moved stuff from Spectator to abstract DomSpectator (for querying, event dispatching, etc) in order to reuse it
  • Created new SpectatorForDirective which contains stuff from both Spectator and SpectatorWithHost
  • it uses the host fixture, so that querying etc is done from host
  • instead of component this one has a directive instance
  • existing code still works (I kept the unit tests)
  • added some tests for the new factory (we should have more of them I think)
  • updated README

const createHost = createHostDirectiveFactory(HighlightDirective);

// calculated styles not supported in JSDOM
xit('should change the background color', () => {
Copy link
Member

Choose a reason for hiding this comment

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

remove xit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I removed it but the tests fail. I had to exclude these tests earlier because the toHaveStyle() matcher is not supported in Jest. It relies on getComputedStyle().getPropertyValue().

Copy link
Member

Choose a reason for hiding this comment

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

There is a way to check if the user uses Jest and warn them that it will not work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently don't have a way to detect it. Let's see if another solution is possible, e.g. not relying on computed styles. Let's fix it in a separate PR.

README.md Outdated Show resolved Hide resolved
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