Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An alternative fix for PR#47 - Add support for numeric values passed to ‘border-spacing’, e.g. ‘border-spacing: 0’ #52

Merged
merged 4 commits into from
Jul 10, 2018

Conversation

lsiu
Copy link
Contributor

@lsiu lsiu commented Sep 27, 2017

This fix include a test and follow the approach used in padding.js

@eddies
Copy link

eddies commented Jun 29, 2018

@lsiu can you rebase this on master? Hopefully that would increase the chances of getting this merged. I have a rebased version of your branch ready at https://github.com/eddies/CSSStyleDeclaration/tree/pr-47-rebase, but I don't want to duplicate your PR

…ng’, e.g. ‘border-spacing: 0’. Include test.
@lsiu
Copy link
Contributor Author

lsiu commented Jul 4, 2018

@eddies - I rebased with upstream/master for this pull request. Now there is no merge conflict.
@jsakas - Seems like a few folks are having the the same problem. Any chance for this PR getting accepted? Any feedback is also appreciated. Thanks.

@lsiu
Copy link
Contributor Author

lsiu commented Jul 4, 2018

If you refer to the w3c border-spacing spec, you can see the "initial value" is 0, not "0", so I suppose a numerical value is also a valid, and should be taken into consideration.

@@ -10,6 +10,9 @@ var parse = function parse(v) {
if (v === '' || v === null) {
return undefined;
}
if (typeof v === 'number') {
v = String(v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think return undefined makes more sense to mimic the current browser behaviour.

@jsakas
Copy link
Member

jsakas commented Jul 6, 2018

@lsiu @eddies @jansiegel

Browsers do not seem to support this functionality. Tested in Chrome, FF, Safari, Edge:

var table = document.createElement('table')
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', 10);
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', '10');
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', '10px');
console.log(table.style.borderSpacing); // 10px

Instead of converting the value, I think it would make more sense to return if the value is not a string.

@eddies
Copy link

eddies commented Jul 7, 2018

@jsakas In Firefox 61.0 (OS X):

var table = document.createElement('table')
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', 10);
console.log(table.style.borderSpacing); // "10px"
table.style.setProperty('border-spacing', '10');
console.log(table.style.borderSpacing); // "10px"

Moreover, in Firefox, Chrome (67) and Safari (11.1.1), I observe the following:

var table = document.createElement('table')
console.log(table.style.borderSpacing); // ""
table.style.setProperty('border-spacing', 0);
console.log(table.style.borderSpacing); // "0px"
table.style.setProperty('border-spacing', '0');
console.log(table.style.borderSpacing); // "0px"

So I don't think a blanket return undefined is the clear winner. I think it's either the PR as-is or a special case for 0 and undefined for other numbers.

EdwardBetts and others added 3 commits July 11, 2018 00:55
The previous `npm test` script executed a `.sh` file
that isn't natively supported by Windows.

Since the actual code inside the `.sh` is pretty small,
it can be completely embedded inside a `npm` script instead,
making it cross-platform and more straight-forward.
@jsakas
Copy link
Member

jsakas commented Jul 10, 2018

@eddies interesting, it seems to work for 0 only, but not for any other integer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants