Class names should be more unique or assignable

[orangecoloured]

I would advise changing class names that are used to something more unique. Or even make them customisable. For example these are pretty common and can be already used in any kind of a project: outer-container, inner-container, content-wrapper.

That's what I encountered. The script just doesn't work because those already have assigned height, width, etc.

[rommguy]

@orangecoloured thanks for opening the issue, you are right.
Any chance you can create a PR for this ?

[orangecoloured]

@rommguy Not sure yet. Fixing it now. Will get back to you later when I make it work :)

[orangecoloured]

@rommguy So, I've got it working. Had to make a bit of refactoring so it would properly pass linting. Probably it's going to be a little bit messy. Anyway, you'll have a look for yourself when I submit it. Gonna do it today. Thanks for the package btw, it's really cross-browser.

[orangecoloured]

@rommguy #45

[rommguy]

Looks like an excellent improvement, thanks!
I'll review and merge in the next few days.

[orangecoloured]

@rommguy, did you have time to review my PR? Are you going to merge it anytime soon or do you want anything changed/fixed beforehand?

[rommguy]

Hi @orangecoloured
I reviewed some of the code and wrote comments in your PR page
All the style changes are a problem, can you try to keep the current style (spaces, semi colons etc.)
Also one syntax mistake - 6x instead of 6px

About the logic -
You added a prop "cssPrefix" but it looks like if someone used it, the entire functionality will break, since all the css selectors will stop working.
How do you solve that ?

[orangecoloured]

@rommguy
Sorry, didn't see your reply.
I've made a little fix and answered the prefix question.

[orangecoloured]

@rommguy Would you like me to fix the css issue?
I can make the styling to be inline instead of using external css rules.

[sag1v]

This is a critical issue in my opinion.

[sag1v]

I'm trying to play with it but i get errors when i try to run build or sassex:

react-custom-scroll\node_modules\minimatch\minimatch.js:116:11)
throw new TypeError('glob pattern string required')

npm run develop works with no errors.

@orangecoloured have you encountered similar issue?

[orangecoloured]

@sag1v are you building from my PR or the master branch?
I had to fix all the syntax errors to make the build.

[sag1v]

@orangecoloured i forked the master branch

[orangecoloured]

@sag1v yeah, then you'll get a huge amount of errors making the build. I think there are two ways of solving this. Either remove this linting from the build process or make the code to comply with its rules. I chose the second way, as I thought there was some reasoning for having that linter there.

[sag1v]

This is not a linting error though.
Just in case, I removed npm run lint from the build script and still get the same error.
It is probably something to do with minimatch and sass.

@rommguy Do you have an idea what's that all about?

[sag1v]

I think the issue related to windows or mac / linux when runing postcss (i'm on windows 10).
postcss/postcss-cli#34
milligram/milligram#122

when i remove the sassex from the build script, it builds fine with no errors.

@orangecoloured on which OS are you?

[orangecoloured]

@sag1v linux.

[sag1v]

I posted a PR #51 using the css-module solution.

@orangecoloured i hope you don't mind 😃

[orangecoloured]

@sag1v not at all.

[rommguy]

Hi guys,
I'll probably merge it in the next few days.
I will also separate the example build from the build script, so you won't have any trouble running the build script.
@orangecoloured the eslint errors are something related to your environment. I don't have any errors when running eslint on the project, that's one of the reasons I didn't want to merge the first PR.