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

Let's agree to a prefix and always prefix mixins when converting #79

Closed
wyuenho opened this issue Nov 3, 2012 · 11 comments
Closed

Let's agree to a prefix and always prefix mixins when converting #79

wyuenho opened this issue Nov 3, 2012 · 11 comments

Comments

@wyuenho
Copy link
Contributor

wyuenho commented Nov 3, 2012

As reported on issue #52, bootstrap mixins have names, parameters and behaviors that conflict with Compass'. However, since that ticket was closed and pull requests merged, maybe because I'm the only person unlucky enough to be affected by this issue, or that looking at the giant diff takes too much effort, I keep seeing newer pull requests coming in without the prefix or use a different one.

In my pull request #53, I opted to use "bootstrap-" as the prefix, but maybe that's not a good one. Can we agree on a prefixed please?

@vwall @tijsverkoyen @kristianmandrup?

@tijsverkoyen
Copy link
Contributor

Personally I think bootstrap- is a fine proposal. It makes it very clear that the mixins are bootstrap related.

@tijsverkoyen
Copy link
Contributor

@wyuenho You removed your comments on my pull-request. But is there a decent way to test the upgrade? Would be nice to see some docs about testing. I just downloaded the zip-file from bootstrap and replaced the generated CSS-file. I know this isn't the best way to test, but I couldn't think of a better way.

@wyuenho
Copy link
Contributor Author

wyuenho commented Nov 3, 2012

Depends on what you want to test, but if you just want to test for the signatures of the mixins, you can just get a list of them from Bootstrap and Compass' and compare them. If compass-twitter-bootstrap is included afterwards, the non-prefixed mixins will redefine Compass' mixins and trying to include them using Compass' signatures will result in either errors or incorrect output.

I'm aware of the difficulty in testing the conversion, so I spent a lot of time looking at the diff before committing, things like converting namespaces, loops and mixin references can be easily spotted this way, but other things are ore subtle. I spent a lot of time looking at the diff in my last pull request but I still managed to slip in a couple of bug, so...

@wyuenho
Copy link
Contributor Author

wyuenho commented Nov 3, 2012

I think bootstrap- is fine too, but it might conflict with other plugins that might also use the prefix bootstrap- to do something else. Maybe that's why @kristianmandrup chose ctb-?

@tijsverkoyen
Copy link
Contributor

Hm, didn't thought about conflicts in other plugins. ctb- is more specific. So +1 for ctb-.

@wyuenho
Copy link
Contributor Author

wyuenho commented Nov 5, 2012

@kristianmandrup and @vwall what say you?

@kristianmandrup
Copy link
Contributor

+1 for ctb- prefix of course. Needs to be concise but specific to this plugin in order to avoid conflicts.

@wyuenho
Copy link
Contributor Author

wyuenho commented Nov 5, 2012

@vwall can you put a note in the README to let people know to always use a prefix in the mixins and stick to ctb-?

@vwall
Copy link
Owner

vwall commented Nov 5, 2012

+1 for ctb-

@tijsverkoyen
Copy link
Contributor

ok. I included the prefix in my pull request: #78

@tijsverkoyen
Copy link
Contributor

I added a section in the README.md, see: #82

@wyuenho wyuenho closed this as completed Nov 17, 2012
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

No branches or pull requests

4 participants