search

KyleAMathews / typography.js

A powerful toolkit for building websites with beautiful design
http://kyleamathews.github.io/typography.js/
MIT License
3.83k stars 182 forks source link

overrideThemeStyles = Broken #104

Open bradennapier opened 7 years ago

bradennapier commented 7 years ago

Returning any value breaks everything. I have tried everything i can think of - looking at your code it woudl appear your demo is broken as you need top return a copied object - this still breaks everything.

ERROR in ./~/css-loader?{"modules":true,"sourceMap":true,"localIdentName":"[name][local][hash:base64:5]","importLoaders":1}!./~/postcss-loader!./src/shared/app/index.css Module build failed: TypeError: Expected a string at module.exports (/home/ubuntu/workspace/node_modules/decamelize/index.js:4:9) at /home/ubuntu/workspace/node_modules/typography/dist/utils/compileStyles.js:36:51 at /home/ubuntu/workspace/node_modules/typography/dist/utils/compileStyles.js:45:11 at /home/ubuntu/workspace/node_modules/lodash/_createBaseEach.js:24:11 at forEach (/home/ubuntu/workspace/node_modules/lodash/forEach.js:38:10)

bradennapier commented 7 years ago

If you return ANY value no matter what it is in overrideThemeStyles I get:

Property:  marginLeft
Property:  marginLeft
Property:  marginRight
Property:  paddingLeft
Property:  fontSize
Property:  lineHeight
Property:  marginTop
Property:  0

The 0 is what breaks it.

PS: I returned options and that errors happens

bradennapier commented 7 years ago

So apparently when editing the theme styles you have to return the styles item and manually modify the already compield styles -- that doesn't make any sense... you should have editing of the options like the normal API and themes do if I return styles it works, if i dont it fails

KyleAMathews commented 7 years ago

Did you see this? https://github.com/KyleAMathews/typography.js#customizing-themes

I can see that overrideThemeStyles isn't perhaps clear enough — perhaps overrideGeneratedThemeStyles instead?

bradennapier commented 7 years ago

Yes I saw that but it seems 100% pointless. I can write my own css from scratch if i want to. But to change the font a theme uses I have to edit like 20 values and hope it doesnt change? Why not allow editing of options. Now theoretically if can be done using ES6 methods easily enough and spread operators but that isnt always an option. I actually just quit using the plugin because it needs es6 and right now postcss-loader wont load postcss config in es6 and i had already spent almost 8 hours trying to make it do what i wanted... had to edit their source code to make it even run

KyleAMathews commented 7 years ago

The theme is just an object — just change a property e.g. baseFontSize — themeObject.baseFontSize = '20px. Also curious what problems you ran into needing es6? Is that the postcss typography plugin? Typography itself is all compiled to ES5.

KyleAMathews commented 7 years ago

Also the postcss-typography plugin is 19 lines of code so you could pull it into your own project and write it as es5 or submit a PR to fix it there.

/cc @BarryThePenguin

bradennapier commented 7 years ago

Well it mostly was due to the fact the postcss plugin does not work with your themes at all because you removed the "scale" function and I would get an error so I was trying random stuff... then all the demos show import style setup but postcss.config is not es6 for some reason even though every single other thing in my setup is... the postcss is called as es5 and there appears to be nothing I could do as when I used require to turn it back into es6 again then everything failed all together so I really don't know - spent the whole day dealing with that.

I did end up just editing the theme object and that did work but if I include the postcss typography plugin it kills all other plugins for me so i just removed it instead as it was taking far too much time.

-- Yeah I know it's fairly straight forward. I was able to fix the plugin by removing the node_modules folder manually at the end of the day. Barry pointed out the beta to me after I had already modified everythign up so I hadn't tried it yet.

KyleAMathews commented 7 years ago

Sorry you had troubles — not familiar with the postcss world. Not sure why you were having trouble with the scale function. It's used in most of the themes.

KyleAMathews commented 7 years ago

Perhaps something about the postcss environment.

bradennapier commented 7 years ago

Yeah, they had it installed as a dependency within their folder and the themes required the new one. So when adding the themes calling scale they didn't have access because you had renamed vr.????? (something about text size) to scale since then. Deleting their node_modules folder fixes it, just took forever to track down the specific cause.

BarryThePenguin commented 7 years ago

If you're using postcss-typography, you'll want to make sure you're running postcss on node 4 or above.

bradennapier commented 7 years ago

I'm running 7.2.0 currently.

KyleAMathews commented 7 years ago

Hmmm... oh right, they must have been using an older version then perhaps. I've been refining the API as we're approaching a 1.0.

On Tue, Jan 17, 2017, 8:16 PM bradennapier notifications@github.com wrote:

Yeah, they had it installed as a dependency within their folder and the themes required the new one. So when adding the themes calling scale they didn't have access because you had renamed vr.????? (something about text size) to scale since then. Deleting their node_modules folder fixes it, just took forever to track down the specific cause.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/KyleAMathews/typography.js/issues/104#issuecomment-273378660, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEVh5cbJ-9Ljn6_BIeGVNEo-Ln5ukabks5rTZIQgaJpZM4LmVGx .