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

assert_dom :text collapses whitespace #123

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jyeharry
Copy link

@jyeharry jyeharry commented Mar 10, 2025

  • Add macos to supported platforms in Gemfile.lock
  • Add failing test for assert_dom collapsing whitespace
  • Collapse whitespace from :text but not :html

Fixes #121

This PR is my preferred solution over #122.

Instead of making use of a :strict operator to determine whether or not to collapse excess whitespace as the browser does, we instead use the existing :html equality operator to match for text with all whitespace included. The :text equality operator is then optimised to collapse all whitespace.

In my opinion, :text should match what would be returned by Element.innerText in javascript land, in that it does not include the excess whitespace. Element.innerHTML on the other hand, does include all of the whitespace that was in the document which I think is what the :html operator should match (which it does already) and so :html could instead be used as a :strict parameter.

The only difference between what Element.innerText and the updated :text equality operator does is that Element.innerText replaces <br> tags with \n, whereas our :text operator just removes them without replacing them. But this is out of our control as it is Nokogiri that does this. Regardless, this is my preferred solution but I made the two PRs since the original issue I raised specifically mentions a strict parameter.

@flavorjones
Copy link
Member

flavorjones commented Mar 10, 2025

There seems to be an issue with the CI workflows related to the Logger gem. I'll fix it on main and then you can rebase this and #122.

@flavorjones
Copy link
Member

flavorjones commented Mar 10, 2025

OK, main is green, so please rebase when you have a chance.

@jyeharry
Copy link
Author

So to confirm, you won't both PRs merged, not just this one? I made the two PRs so that there were a couple of options for how this would be implemented, with the intention that only one would get chosen. Let me know what you think is best.

I should probably also update the commented docs just before the assert_dom definition to make the updated behaviour of :text and :html a little more clear.

@flavorjones
Copy link
Member

flavorjones commented Mar 14, 2025

So to confirm, you won't both PRs merged, not just this one

I just want whatever you want to submit rebased so I can see if the tests pass, please! Or let me know if you want me to do it, that's fine, too.

I'm sorry I've been busy this week, but in general I want to see:

  • green CI (tests passing)
  • then I will look at the code and review it
  • then we can make a decision about merging

and we're still on that first item until the branch is rebased on the current main. I hope that makes sense and clarifies? 🙏

@jyeharry
Copy link
Author

Ahhh right sorry for some reason I read "rebase" as merge the pull request. I'll rebase now.

@jyeharry jyeharry force-pushed the feature/assert_dom-ignore-whitespace-v2 branch from 27f1469 to c052922 Compare March 15, 2025 02:47
@jyeharry
Copy link
Author

@flavorjones Rebased and tests and ci is passing

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

Successfully merging this pull request may close these issues.

assert_dom should ignore whitespace just like assert_dom_equal
2 participants