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(logDOM): add logDOM export #336

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Conversation

kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Aug 9, 2019

What: add logDOM export

Why: I was working on #314, but I misunderstood it and changed gears. But I liked the refactor I was working on it.

How: Moved some logic around and now pretty-dom is responsible for determining default values for limit and options. I think it's a bit cleaner this way.

This is a breaking change because it basically moves debugDOM logic to prettyDOM. debugDOM was pretty useless after this change, so it's been removed.

It also adds an export for a new logDOM which does what it says.

Checklist:

  • Documentation added to the
    docs site N/A
  • Typescript definitions updated N/A
  • Tests
  • Ready to be merged

@kentcdodds kentcdodds closed this Aug 9, 2019
@kentcdodds kentcdodds reopened this Aug 9, 2019
@kentcdodds
Copy link
Member Author

Actually, it looks like debugDOM wasn't exported at all anymore and now it is. So this is a new feature. I like it :)

@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #336 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #336   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     21           
  Lines         311    315    +4     
  Branches       66     66           
=====================================
+ Hits          311    315    +4
Impacted Files Coverage Δ
src/query-helpers.js 100% <100%> (ø) ⬆️
src/role-helpers.js 100% <100%> (ø) ⬆️
src/pretty-dom.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b504ad...1d2d56b. Read the comment docs.

@kentcdodds kentcdodds force-pushed the pr/refactor-pretty-dom branch from 43b3cc1 to 4597576 Compare August 9, 2019 16:53
if (htmlElement.documentElement) {
htmlElement = htmlElement.documentElement
function prettyDOM(
dom = getDocument().body,
Copy link
Member Author

@kentcdodds kentcdodds Aug 9, 2019

Choose a reason for hiding this comment

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

If someone's not running in an environment where the document is globally available, then they have to provide a DOM node to log, otherwise we'll just default to document.body which I think is a good default. I imagine lots of people will just call logDOM() and that'll be helpful to them!

@kentcdodds kentcdodds force-pushed the pr/refactor-pretty-dom branch 2 times, most recently from ba3ef0f to 56a5a16 Compare August 9, 2019 16:58
@kentcdodds
Copy link
Member Author

Dang it. I just realized that we did export debugDOM, but it only returned a string that was formatted the way that I now have prettyDOM doing. So it wasn't that useful...

So here's what I'm thinking. Let's call this a breaking change (because it technically is) and we'll release this one with the RTL change here: testing-library/react-testing-library#430

Thoughts?

@kentcdodds kentcdodds force-pushed the pr/refactor-pretty-dom branch from 56a5a16 to 430d449 Compare August 9, 2019 17:01
@@ -20,7 +20,7 @@ export type AllByAttribute = (

export const queryByAttribute: QueryByAttribute
export const queryAllByAttribute: AllByAttribute
export const debugDOM: (htmlElement: HTMLElement) => string
export const logDOM: (htmlElement: HTMLElement) => void
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the breaking change.

Impact is minimal because people probably aren't relying on this for their actual tests and it's more of a workflow method (which I doubt many people use).

That said, some libraries built on top of DTL may use this method, and the upgrade strategy for them is to switch from debugDOM to prettyDOM.

@kentcdodds kentcdodds requested a review from alexkrolick August 9, 2019 17:03
@kentcdodds kentcdodds changed the title chore: refactor pretty-dom stuff feat(logDOM): add logDOM export Aug 9, 2019
@kentcdodds kentcdodds force-pushed the pr/refactor-pretty-dom branch from 430d449 to 27c9ddd Compare August 9, 2019 17:25
@kentcdodds kentcdodds force-pushed the pr/refactor-pretty-dom branch from 27c9ddd to 1d2d56b Compare August 9, 2019 17:25
@kentcdodds kentcdodds merged commit 5bcd1a7 into master Aug 9, 2019
@kentcdodds kentcdodds deleted the pr/refactor-pretty-dom branch August 9, 2019 19:40
kentcdodds pushed a commit that referenced this pull request Aug 9, 2019
There was an issue with a major release, so this manual-releases.md change is to release a new major version.

Reference: #336

BREAKING CHANGE: If you were using `debugDOM` before, use `prettyDOM` instead.
@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants