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

Update: Adds recursive check for isEmptyRender #1924

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@sstern6

sstern6 commented Dec 4, 2018

Fixes #1866

const emptyNodetestData = emptyNodes();
const nonEmptyNodeData = nonEmptyNodes();
itWithData(emptyNodetestData, 'when renderedDive receives nodes that render validEmpty values it returns: ', (data) => {

This comment has been minimized.

@ljharb

ljharb Dec 4, 2018

Member

i'm not sure i'm a fan of generative test cases - i'd rather have them all laboriously written out, or, a hardcoded test that iterates through a hardcoded array of data.

This comment has been minimized.

@sstern6

sstern6 Dec 4, 2018

Sounds good, I had originally done that but saw the generative test pattern so I went with that. Ill fix.

const renderedKey = 'rendered';
function dive(renderedNodes) {
const isList = renderedNodes && renderedNodes.length > 0 && Array.isArray(renderedNodes);

This comment has been minimized.

@ljharb

ljharb Dec 4, 2018

Member

= Array.isArray(renderedNodes) && renderedNodes.length > 0?

return renderedNodes.some((n) => {
if (n) {
if ((has(n, renderedKey))) {
dive(n.rendered);

This comment has been minimized.

@ljharb

ljharb Dec 4, 2018

Member

should this be n[renderedKey]?

This comment has been minimized.

@ljharb

ljharb Dec 4, 2018

Member

or maybe we don't need renderedKey at all, and we can just hardcode .rendered

if (n) {
if ((has(n, renderedKey))) {
dive(n.rendered);
} else if (!validEmptyValues(n.rendered)) {

This comment has been minimized.

@ljharb

ljharb Dec 4, 2018

Member

also here?

This comment has been minimized.

@sstern6

sstern6 Dec 7, 2018

if we remove renderedKey could we keep this?

This comment has been minimized.

@ljharb

ljharb Dec 7, 2018

Member

yeah def; we should either use renderedKey everywhere, or nowhere

return isEmptyRender;
}
dive(nodes);

This comment has been minimized.

@ljharb

ljharb Dec 4, 2018

Member

could this not be return dive(nodes)?

This comment has been minimized.

@sstern6

sstern6 Dec 4, 2018

So initially I did have that. And then in my utils test cases, I was getting inconsistent return values. I will change back, bc this is something I prefer and research why.

@ljharb ljharb added Mount PATCH labels Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment