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

Placeholder classnames doesn't seem to work #24

Closed
jameswlane opened this issue Nov 15, 2017 · 9 comments
Closed

Placeholder classnames doesn't seem to work #24

jameswlane opened this issue Nov 15, 2017 · 9 comments

Comments

@jameswlane
Copy link
Contributor

  • glamor version: 2.20.40
  • jest-glamor-react version: 3.2.1
  • node version: 8.9.1
  • yarn version: 1.3.2

What happened:
When running snapshot testing the classnames are not being replaced with placeholder classnames.

Reproduction repository:
https://github.com/jameswlane/jest-glamor-react-issue

Problem description:
My snapshots are using glamor classnames

exports[`<LineScaleRandom> <LineScaleRandom> Mount 1`] = `
<LineScaleRandom
  loading={true}
>
  <div
    className="css-1jz1ji0"
  >
    <div />
    <div />
    <div />
    <div />
  </div>
</LineScaleRandom>
`;
@kentcdodds
Copy link
Owner

Hi @jameswlane,
That's odd. I'm not sure what's going on. Could you dig a little deeper to figure out what the problem is? Perhaps dive into your node_modules and put a few console.logs around?

@jameswlane
Copy link
Contributor Author

@kentcdodds,
I can do that, It was getting late so I thought I would pop it on here to get some visibility on it. Maybe it was something that's been seen before. I will dig around tonight starting here and see what I can come up with.

@jameswlane
Copy link
Contributor Author

@kentcdodds, figured out the issue that was effecting the placeholder classnames.

Style that effected filter:

'> div': {display: 'inline-block'}

Style that worked:

' > div': {display: 'inline-block'}

The missing space before the > was the culprit.

It would work fine in the browser but when the filter did the comparison it would fail.

return rule.selectors.some(selector => {
  const baseSelector = selector.split(/:| |\./).filter(s => !!s)[0]
  return nodeSelectors.some(
    // sel:  .css-yap912 === css-yap912>   returns FALSE
    // sel:  .css-yap912 === .css-yap912>  returns FALSE
    sel => sel === baseSelector || sel === `.${baseSelector}`,
  )
})

@kentcdodds
Copy link
Owner

Awesome! Thanks for finding that! Do you think you could make a pull request to fix it (including tests)?

@jameswlane
Copy link
Contributor Author

I can look at it this weekend.

First I want to test it with a number of multiple selectors:

> // Child
~ // Siblings
+ // Adjacent
a:visited // Pseudo-classes 
p::first-line // Pseudo-elements

See how glamor will handle them and what ones could cause a failure.
Then we could look at the end of the baseSelector and if a selector exist we could strip it off for the comparison.

@kentcdodds
Copy link
Owner

Sounds super! Anything you can do to improve things in this regard would be fantastic 👍

@jameswlane
Copy link
Contributor Author

I am thinking something as simple as this should get everything.
I plan to make sometime today to TDD it out.

searchText.match(/[^>|~|+|:]*/)

@jameswlane
Copy link
Contributor Author

Ran into a few issues this weekend, I could use a second set of eyes.

Here is what I found out:

Pseudo-classes and Pseudo-elements do not effect this bug, so we can not worry about them.

a:visited // Pseudo-classes 
p::first-line // Pseudo-elements

As for the child selector:

> // Child

I added the following to test here, but my regex seems to not be very strong this weekend. I have yet to find a regex that will match and allow me to select everything before the selector.

  {
    title: 'child',
    styles: {
      '> div': {
        display: 'inline-block',
      },
    },
  },

Last one that's throwing me off:

~ // Siblings
+ // Adjacent

I added the following to test here and its throwing a Error: undefined:1:723: missing '}' that has me at a loss.

  {
    title: 'siblings',
    styles: {
      '~ div': {
        display: 'inline-block',
      },
    },
  },
  {
    title: 'adjacent',
    styles: {
      '+ div': {
        display: 'inline-block',
      },
    },
  },

@kentcdodds
Copy link
Owner

Why don't you go ahead and open a pull request with what you have and we'll see what we can do about it :)

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

No branches or pull requests

2 participants