-
Notifications
You must be signed in to change notification settings - Fork 249
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Braxton Fair
committed
Jun 22, 2016
1 parent
654bb6d
commit 7642e56
Showing
2 changed files
with
5 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp... you definitely did manage to break my build :)
No big deal, I'll just use 6.1.2 for now.
FYI: since I'm using Jeet in prod, I'm willing to invest some time in improving it.
If you create easily actionable issues, I'll help out.
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But this really should've been version 7.0, since it has breaking changes: http://semver.org/)
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it now, just pushing out a release with the ton of API changes in it (since 6.1.2, a year worth of changed stuff) probably wasn't the best idea.
@x1024 can you tell us what the specific issues are, maybe even detailed in an issue if there's more to it? If you have problems installing it then I might suggest updating your used node-version (even thought it should work with almost any version).
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two issues:
I was using "@import jeet/stylus/jeet".
Since index.styl was removed, that no longer works.
I was using the non-prefixed jeet functions. So "column" and "col" instead of "j-column"
There's no need to prefix something like that, who uses multiple different grid systems in the same project?
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, seems multiple people that also used the development version had problems, there are several issues about this.
I'm not really fond of Stylus myself, so I need to look into it a bit to find a way fixing this issue.
Pretty simple reasoning, as prefixing was introduced as Stylus already has
shift
/unshift
functions (They even broke the tests), so they added aj-
prefix to the API andjeet-
to all private functions.I will update the API reference for now, showing these changes and giving a warning on the page that the general syntax has changed.
We might think of a proper way of prefixing for 7 with aliases (so there is a fallback for old versions), cleaning up stuff that was left behind.
We just took over the maintenance of the project in the recent days, so it still takes a bit getting into it and of couse, getting the old stuff sorted out, there hasn't been happening stuff for quite a while. So I'm sorry for the inconvenience caused at the moment.
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only solution I can think of is having an index.styl that imports _jeet.styl.
There could be a _jeet_unprefixed.styl file that imports _jeet.styl and defines the old aliases.
Alternatively, the unprefixed functions could be kept as the default (in a 6.1.4 that will un-break the code of 6.1.3 users), and there could be a _jeet_prefixed.styl
It's alright, I understand the project is "under new management"
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at it, I can simply wrap up aliases in SCSS, while in Stylus it is a bit more work to achieve, but I'll do that for now.
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@x1024 just opened a PR (#477) with the first set of prefixes for SCSS. As you are using Stylus you might know how to achieve the same thing with that, so if you could help me out with this, that'd be great!
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I received an email for the PR.
Would you prefer to have "column aliased to j-column" or "j-column aliased to column"?
If our goal is to prevent conflicts, I think the prefixed version should be the default, and the unprefixed aliases should be in a separate file.
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
column aliased to j-column
and I found a way doing stuff with Stylus, which I'm currently testing, so I'll (hopefully) push it into the PR soon
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#478 please review if the SCSS version works. I tested the Stylus version.
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@x1024 just tested it, it works fine!
7642e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the problem too with the @import 'jeet'; in stylus after that commit.
Yesterday worked fine, but today with a npm install broke.