Add nth-of-type support, optional args order #216
Conversation
|
I'm sorry that I haven't gotten to this just yet. Trying to iron everything else out before diving into new things. I shouldn't have promised to get back yesterday. It might be more like mid next week. I want to fully go through all the existing code with a new eye and fine-tooth comb to understand and have better opinion about it. I'm really like this PR, though. The build selector and args are really clever. I think this is a really nice foundation for enabling a lot of cool things to come. Thanks for taking the time to get this PR in. |
|
Yeah, thanks @zgreen ! 👍 |
|
@zgreen Aright, I'm going to dive into this this week. Thanks for your patience! |
|
No problem! Happy to help if it needs more work (more tests, maybe?) |
|
From the quick glance I did tonight, this is doing some epic things. I'm liking the the I love that you're using ES2015 but I think that might have to come at a later time. I want to think about this a bit more. I need to head to bed right now, but will digest and get back. |
|
Cool cool. I tried briefly to get Babel 6 going on this, but the setup is pretty different from Babel 5 and I ran into some issues. I could probably sort those out, if you want. |
|
I am hesitant to only have some of the codebase on ES2015. I'd rather do a larger migration of the entire codebase to it, possibly also with typing? I'm open to others thoughts here. |
|
@zgreen I've been thinking about this a lot. This code is epic and I don't want to disrespect it and your time and I think what you have here is three amazing features. I think there is enough that is touching the core that I'd like to push at least the optional arguments order feature to 7.0 as there is a lot of core work there. I think the nth-child would be a powerful feature and would love to pursue that before 7.0 but you know whether or not the two could be separated easily. Re: ES2015. I think for right now lost shouldn't move to it just yet. Maybe in 7 or possibly 8. I would like that to be a large overhaul that may even involve typing as well. This is open-source though, just cause I have the |
|
@peterramsing This is the difficult part about maintaining. Sometimes people will have enough time to PR small bits of great code and if they don't have the time to do the rest, it's up to you to finish the job or leave that good bit of code hanging. If you're fortunate, it's usually just busy work. If you're not, then it can get pretty hairy. Maybe it's worth merging small changes like this, and then having a detailed Issue label like "Help Wanted: Busy Work". A lot of times people who are new to contributing to open source will jump on stuff like that just to get some coding/GitHub practice - especially on a pretty popular project. |
|
Yeah, I certainly don't want to make @zgreen go back and code a bunch more! (sorry @zgreen for it feels like we're talking behind your back 😄) If I'm understanding you, @corysimmons, you're saying that this looks pretty good to merge and then I should do the changes that I feel are necessary after the merge (or label them as Help Wanted)? If 6.70 is scoped to just this and #218 I think this could be a pretty great version and I can spend from now till then polishing it. I'm going to let this sit one more night and ponder it on my run tomorrow morning. |
|
When projects are younger, you can get by with merging things all willy-nilly, but once they get pretty big like this you gotta be more careful. It seems like this PR introduces some good changes, but only to a few files, why not backburner it until you have a free evening to go through it and add some commits to make all the changes standardized across every function? In lieu of that, I'd backburner it and slap In lieu of either of those things, make sure it's passing tests, and do some visual testing on it to make sure it doesn't break anything the tests are missing, then merge and create a new Issue to catch the rest of the codebase up with the changes introduced in this PR. |
|
Hi, I was looking over this PR and noticed that it will break EM and REM support for gutters. The offending code is if (declArr[i].indexOf('px') !== -1 || parseInt(declArr[i]) === 0) {
lostArgs.gutter = declArr[i];
} |
|
@poxrud Ah, good catch! |
|
Sorry, work got in the way and I'm just getting back to this. I think 7.0.0 makes sense for this PR. I mainly added the optional args order because it seemed like having to pass five arguments to Re: ES2015, I can remove those bits fairly easily if needed, I think it's almost (or all) Either way, I'd like to add some more tests, and fix the issue @poxrud noted. I'm happy to continue to put in some work on this, but likely will not be able to until next week, at the earliest. Thanks! |
|
No worries @zgreen! There is no rush. Let's slate this for 7.0.0, then. You're welcome to do whatever work you'd like but before you go crazy coding I think we should define 7.0.0 a bit more to fully understand all the features a bit more. I don't want to make extra work. :) I've been sick the last few days and am on holiday tomorrow so 7.0.0's definition might be later in the week/next week but I do need to get on that. So many people wanting to help! I suppose this is a great problem to have. This is the first project I've managed with multiple people not in an office and I can't see the daily commits of people. I want to make sure people's time is respected. 😄 Thanks again @zgreen for everything! |
|
Closing for now. This should be in 7.0.0 - and see #237 for some of the talk about syntax...including ES2015. 😄 |
|
In theory this could be resurrected... gonna re-open to make sure it's not dropped |
See #214