-
Notifications
You must be signed in to change notification settings - Fork 118
duo-css: should parse each of the css files in "styles": [...] #36
Comments
Is this for backwards compat with Component or for regular Duo stuff? I can outline how I was hoping the CSS stuff would work if that's helpful |
i have a good idea of how CSS will work within an app. Basically just point to a single yah, if you have some ideas, i'd appreciate it. trying to figure this stuff out now. backwards compat is necessary, but we can also do stuff that will only work in duo |
What are the other distinctions that we have between "apps" and "components"? I think it's just that What I imagined was that both cases would have a CSS entry point file, defaulting to
...and then for packages, they would have a {
"name": "toggle",
"repository": "segmentio/toggle",
"main": {
"js": "lib/index.js",
"css": "lib/index.css"
}
} ...and the same rules would then apply. Makes it easy, since to maintain backwards compat for existing components, we just need to generate an entry point file with imports to everything in fs.writeFileSync('index.css', styles.map(function(file){
return '@import "' + file + '"';
}).join('\n')); ...and I think that should cover it? |
yep, this works for me. then we'll check |
Yup, and I even just realized we can nicely check the extension if it's a string for even more convenience for single-use packages: {
"main": "lib/index", // { js: "lib/index.js" }
"main": "lib/index.js", // { js: "lib/index.js" }
"main": "lib/index.css" // { css: "lib/index.css" }
} But that might have been obvious to everyone else already :) |
ohh yah, <3 |
@ianstormtaylor I don't think this is going to work for components that depend on existing CSS dependencies. I'm not sure how many of them there are, but in order for a component's CSS to depend on CSS from another component in duo, it would need something like: component/tip: @import "aurora-tip"
Which is definitely not backwards compatible with component. This is probably an edge-case, but I've noticed you've done a lot of CSS components, so I wanted to run this by you. |
oh... the you think that's going to be a problem? /cc @yields |
Just running into this issue again. Was trying to use one of our existing components and its |
Yah... this was a compromise that I'm hoping we can get away with. I think you guys are the biggest and perhaps the only pure CSS component (with deps) users. The solution I'd like to propose is to update the CSS components to do: // in index.css @import "logo/logo"; ... This has the added benefit of only including the CSS that you need instead of adding all the CSS files in the styles array. I'm down to help with this transition but I know this ends up breaking component compatibility so let me know if this is not feasible and we can work around it. Sent from my iPhone
|
Ah nice I like that interim fix with the |
oh i thought that would be a permanent fix haha. what do you have in mind? |
Ah oops I meant interim as in until we get rid of component altogether. Just liked that it was backwards compat still in the interim as we switch over |
oh shoot, the rest of the message got cut off becuase i was messaging on my phone... dunno if you saw: ... This has the added benefit of only including the CSS that you need instead of adding all the CSS files in the styles array. I'm down to help with this transition but I know this ends up breaking component compatibility so let me know if this is not feasible and we can work around it. Sent from my iPhone |
Ah nice, yeah that's also cool. Been digging the total control over ordering of deps in css land compared to before. way more sensical :) |
yep haha :-) |
@ianstormtaylor @matthewmueller fyi this actually won't work because check it out: That seemed like an easy fix. This way, component pulls from However, this doesn't actually work as expected (maybe I just misinterpreted from the beginning). Duo first checks It seems like it should work the other way around, if we're not going to be supporting CSS in the manifests (#252), yeah? Basically, duo should first check |
oh yah shoot, An easy fix would be: {
"main": "index.css",
"styles": [
"style.css"
]
} or... {
"styles": [
"index.css",
"style.css"
]
} I'm not sure I want to support this... it would require an |
@matthewmueller ok cool, the |
@matthewmueller gahh, nevermind it won't do it because we already have {
"styles": [
"index.css",
"style.css"
]
} that wouldn't work because in |
@lancejpollard duo adds support for: {
"main": {
"css": "index.css",
"js": "index.js"
}
} for multi-asset components. |
@matthewmueller any reason why you don't want that extra does component support that? "main": {
"css": "index.css",
"js": "index.js"
} |
arg, we need more docs haha. ian simplified / improved the docs a lot, but we also lost a bunch of info. that was originally in the components section of the docs. going to add a "creating a duo component" section. |
I think the It seems like there isn't a simple workaround for these, so maybe we should add support to Duo core to properly create an |
oh yah, good point. yah i don't think there's a great way to add support for both :-/ |
alright i guess for now we'll probably just fork duo and go from there. would you accept a pr or no you don't want to support this? basically it doesn't seem there is anyway we can both use duo and component at the same time while we refactor/port stuff over without this. some other workarounds other than forking duo are:
|
Would the exists check fix it? Basically the order of preference would be:
I just wonder if it's a good idea to ignore the manifest if it's there. As I've mentioned a few times before, the The only components that need to get updated are the CSS components that have CSS dependencies. Do you guys have a lot of them? |
@matthewmueller is there a way we can maybe make a plugin or something to hack around this? |
@matthewmueller I guess what's partly confusing for me is why you're supporting the manifest for CSS at all? The way it works now it pretty much doesn't work for any of the components we have other than the simple ones that have What I'm saying is, |
Okay, haha. Umm... yah actually you might be able to: duo.use(function(file, entry) {
if ('css' != file.type) return;
if ('css' != entry.type) return
var json = require(join(dirname(file.path), 'component.json'));
if (!json.styles || !json.styles.length) return;
for (var i = 0, style; style = json.styles[i++];) {
file.src = "@import " + style + ";\n" + file.src;
}
}); This is untested, haha. But that should get the point. |
@lancejpollard the CSS manifest is for new components, not old ones. Duo has a much more powerful CSS component system now, as it will only include the stuff that you need, just like JS and it will also respect order: index.css: @import "normalize.css";
@import "grid";
// ... component.json: {
"name": "css-component",
"dependencies": {
"necolas/normalize.css": "*",
"some/grid": "*"
}
} Using the component: @import "matthewmueller/css-component"; |
@lancejpollard try and get the plugin working, if you have any questions let me know. I can take a crack at it too. It will be important for the transition. |
@matthewmueller for sure, that totally makes sense. That's how we've been doing it definitely. But right now, here's the order of precedence. Here's an existing CSS component {
"main": "index.js",
"styles": [
"lib/index.css",
"lib/form.css",
"lib/form-legend.css",
"lib/form-input.css"
],
"dependencies": {
"org/form": "*", // which requires normalize.css, perhaps
"org/input": "*"
}
} Porting to duo you make it like this (same component.json, but just new {
"main": "index.js",
"styles": [
"lib/index.css",
"lib/form.css",
"lib/form-legend.css",
"lib/form-input.css"
],
"dependencies": {
"org/form": "*", // which requires normalize.css, perhaps
"org/input": "*"
}
} @import './lib/form';
@import './lib/form-legend';
@import './lib/form-input';
@import 'org/form';
@import 'org/input'; It should work like this imo:
Basically, duo isn't currently supporting existing components anyways, so might as well change the order of precedence. |
kk will try the plugin method |
ah yah, i think we can get that working. here's more docs on https://github.com/duojs/duo/blob/master/docs/api.md#duousefngen |
I think this will allow us to continue to support components with JS and CSS. It's a little weird though, because there should probably only be one
style
that imports everything else.@yields @ianstormtaylor @visionmedia could use some thoughts here.
The text was updated successfully, but these errors were encountered: