jsdom / cssstyle

A Node.js implementation of the CSS Object Model CSSStyleDeclaration interface
MIT License
109 stars 70 forks source link

Patch padding margin #38

Closed dpvc closed 8 years ago

dpvc commented 8 years ago

This pull request adds login to set padding and margin when one of its sub-properties changes (and all four sub-properties are set). This should resolve issue #37. It also adds a new test to check that it does what it is supposed to. I also fixed some tab-versus-spacing.

chad3814 commented 8 years ago

Does this only work if you specify all four properties to a margin: or padding:? I'll pull and mess around today

dpvc commented 8 years ago

If all four of padding-top, padding-right, padding-bottom, and padding-left are set when one of them is specified, then the padding style that combines them should be set to the combined value (and the individual values should not show in the cssText string, just the combined one). It does require all four individual values to be set for that to happen.

For example, of you did

style.padding = "1px 2px 3px 4px";
style.paddingTop = "10px";

then style.padding should show 10px 2px 3px 4px and style.cssText should be padding: 10px 2px 3px 4px not something like padding: 1px 2px 3px 4px; padding-top: 10px. But if you did

style.paddingTop = "10px";
style.paddingBottom = 20px";

and the paddingLeft and paddingRight are not specified elsewhere, then padding will be blank and style.cssText should be padding-top: 10px; padding-bottom: 20px.

Does that answer your question?

chad3814 commented 8 years ago

Looks like I missed in your earlier PR but you can't set padding or margin to 0 anymore. That's a simple fix in their isValid(), I'll commit that and make a new test for it.

One thing is, we need to cascade removeProperty() to take out the subproperties.

style.padding = 0;
style.paddingLeft = '1px';
style.cssText; // 'padding: 0px 0px 0px 1px;'
style.removeProperty('padding');
style.cssText; // ''
style.paddingLeft; // '1px'
style.paddingTop; // '0px'

So need to remove all the sub-properties. I think I'll merge this and file an issue for sub-property removal.

What do you think about taking over this project @dpvc ?

dpvc commented 8 years ago

One thing is, we need to cascade removeProperty() to take out the subproperties.

Good point. I'm afraid I don't know what everything is supposed to do, so didn't think of that.

What do you think about taking over this project?

Sorry. I've got too much on my plate as it is. Wish I could do more.

chad3814 commented 8 years ago

no worries, 0.2.32 is published with this fix

dpvc commented 8 years ago

Great! Thanks!