Closed JuanCaicedo closed 4 years ago
There might be a better way to get this, right now I wrote myself a helper function
function getFullWidth(el) {
var rightMargin = el.getRawStyle('margin-right');
rightMargin = rightMargin.replace('px', '');
rightMargin = parseInt(rightMargin);
var leftMargin = el.getRawStyle('margin-right');
leftMargin = leftMargin.replace('px', '');
leftMargin = parseInt(leftMargin);
return el.width.plus(rightMargin).plus(leftMargin);
}
This would also need to handle em
instead of px
to be more robust.
Looks good. This is definitely something worth adding. I'm not sure about the 'fullWidth' descriptor, though. Would body.width.minus(second).minus(second.marginWidth)
be okay, or is that too wordy?
Also, I believe getRawStyle() converts everything to px
, so em
should already be handled for you. (It's using getComputedStyle() under the covers, which does the conversion for us.)
@JuanCaicedo BTW, there's a bug in your helper function: var leftMargin
is getting margin-right
. Not sure if that's in your real code or not, but thought you'd want to know. Otherwise, it looks correct.
Copy paste error 😅
@jamesshore marginWidth
sounds good to me. I could see borderWidth
and paddingWidth
all being useful as well. What do you think of the equivalent height properties?
I can take a stab at implementing this and add any questions I have 😄
Agreed on padding. I'm not sure about borders as they have special cases, but it might be worth doing. I'd prefer to take each one as a separate pull request in any case.
Let's think this through a bit more first... eventually, when we do borders, we will want to be able to say things like border.color
, border.style
, border.cornerRadius
, etc.
So, for consistency, perhaps what we need here is:
What do you think? Am I missing anything?
Some more ideas for margin
--would it be useful to talk about margin's positions? E.g., margin.left.left
corresponds to the left edge of the left margin and margin.left.right
corresponds to the right edge of the left margin?
If so, then we should probably use margin.left.width
rather than margin.width.left
.
I think I lean towards being able to split border.right
and border.left
. I can imagine a case where your border is different on both sides and you want to test them separately. Then padding and margin would be the same for consistency.
I would then imagine that border.width
is the sum of border.left.width
and border.right.width
If we pursue the above, I'm not sure how to handle border.top.width
or border.right.height
I'd say border.top.width
and border.right.height
are just undefined.
Although I'm not sure about border.width
being the sum of border.left.width
and border.right.width
. If your CSS defines border-width: 3px
then it applies equally to all sides. In that case border.left.width
is 3px and border.right.width
is 3px. Having border.width
equal 6px would be confusing.
Perhaps it should be called border.totalWidth
.
That makes sense, I'm good with border.totalWidth
and border.top.width == undefined
My guess is that these, along with the logic for getting them, would need to be added in https://github.com/jamesshore/quixote/blob/master/src/q_element.js#L32?
The proper way to implement this is to create a new descriptor. This guide explains how.
Also, your descriptor should extend SizeDescriptor
. The best example to follow is ElementSize.
@JuanCaicedo I've been thinking about margins. I think there might be a better option.
The problem with margins is that they're an implementation detail. The idea of Quixote is that it allows you to talk about the visual appearance of your page. From a visual perspective, margins don't exist, at least not in the same way that they do in CSS. (Borders and padding, in contrast, do have a direct correspondence in the page.)
Instead of margins, the viewer sees white space. Margins are a way of achieving white space, and because they collapse and combine there's no direct visual connection between the margin
property in CSS and the white space seen on the page.
As a result, I think we might be better off providing ways to talk about white space rather than margins. Specifically, the distance between elements.
I'm proposing something like this: first.right.to(second.left)
. That would be the amount of space between the first
and second
elements.
Then, to talk about how two columns fill up your body element, you might say this:
// first and second fill up the entire body
body.assert({
left: first.left,
right: second.right,
width: first.left.to(second.right)
});
// There's a ten-pixel gutter between first and second
// (This uses a new assertion API that I've been planning)
first.right.to(second.left).should.equal(10);
We might even want to provide a convenience API for this:
body.should.be.filled(first, 10, second); // element, gutter, element
What do you think?
I think I'm on board. I can picture this being useful in addition to the border stuff we're discussing above. For example, when I implemented the styles for the categories section of this page, it would have been nice to be able to make an assertion about how much space there was from the right edge of the text to the left edge of the border.
It seems like what would need to be implemented is a new function to
on any positional descriptors (top
, right
, bottom
, left
). The result of this would eventually be a number, so it could be asserted on width
and height
.
Are you writing keeping track of your thoughts for that new assertion API in an issue? I might like to drop some thoughts in as well 😃
Good idea. See #47.
Regarding the to()
function, I'm planning to implement it on PositionDescriptor
, just like plus()
and minus()
are. That should automatically make it available on all positional descriptors. It will return a SizeDescriptor
(which I might rename), which is the same class width
and height
descriptors use.
@JuanCaicedo I've updated the Descriptor tutorial. The changes are mostly minor but you might find it useful.
@jamesshore I've been thinking about this some more, and I'm not sure it will exactly work for the case I'm trying to test. If you think about a case like the one I show in this codepen, I think an assertion like the following would fail:
parent.assert({
width: first.left.to(second.right)
})
I think this would actually also fail if you manually factor in the width of the margin (since it takes up width on both the left and right.
parent.assert({
width: first.left.to(second.right).plus(second.margin.width)
})
To fully account for this, you would need to factor in both margins
parent.assert({
width: first.left.to(second.right).plus(second.margin.left.width).plus(second.margin.right.width)
})
This makes me think that to
might not be as good of a solution as we were hoping. I agree with coming up with a way of testing the end result instead of the implementation details, but maybe we need to still think of what that looks like.
What are your thoughts?
@JuanCaicedo I'm not following. In plain English, what about that codepen are you trying to check? Is it the gap between the green box and the red box, or something else?
The same as the example above, that the whole width of the parent is taken up by the two children
Except it's not, is it? There's a five-pixel gap between second
and the right hand side of the parent. Unless I've misunderstood things?
|--------------------- parent -------------------|
|------- first -------| 5px |--- second ---| 5px |
first.left.should.equal(parent.left);
first.right.to.second.left.should.equal(5);
second.right.to.parent.right.should.equal(5);
// or maybe...
parent.should.be.filled(first, 5, second, 5)
Yeah, I think we have the same understanding. I just don't think that this type of assertion should require knowledge about the margin, that seems like testing implementation details
I'm still confused--what knowledge of the margin is involved in the example I gave? There's knowledge that there's a gap, but that's a visual feature that presumably you'd want to test.
If you just want to test that the two elements fill the entire parent, with a five pixel gap on the right, you could do it this way:
parent.assert({
width: first.left.to(second.right).plus(5)
});
Ultimately, the child elements don't fill the parent--there's that gap on the right--so any test that says they do should fail.
first.left.to(second.right).should.equal(parent.width);
// Outputs:
// Distance from left edge of 'first' to right edge of 'second' should be 5px larger.
// Expected: 200px (width of 'parent')
// But was: 195px
Okay, now it makes sense 😃 I was thinking that you'd want to avoid needing to specify 5 so that you're testing at a higher level, but your reasoning makes sense. You should have to specify that gap, because it's what the user is experiencing
PositionDescriptor.to() has been added and will be included in the 0.13 release.
Given that PositionDescriptor.to() was added a few years ago, the new assertion API described in issue #47 is coming soon with the v1.0 release, and issue #58 covers additional improvements to assertions, I'm going to consider this one closed.
I want to have a test case where I want to assert that an element takes up the full width of the parent minus the width of its sibling.
However, this breaks when the second element has a margin, because margins are not included as part of the width.
My solution involves manually factoring in the width of the padding, but it would be nice to have something along the lines of