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

Identifying compositions by handler URI #123

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

PatrykSzwer
Copy link
Contributor

@warpech warpech force-pushed the handler-uri-as-composition-identifier branch from df3b090 to 180f332 Compare April 24, 2019 20:37
otherwise many WCT tests of starcounter-include fail
@warpech warpech force-pushed the handler-uri-as-composition-identifier branch from 180f332 to 411a40a Compare April 24, 2019 20:42
because the changes in source code rely on it to work and the test fails because of an outdated fixture
starcounter-include.html Outdated Show resolved Hide resolved
starcounter-include.html Outdated Show resolved Hide resolved
starcounter-include.html Outdated Show resolved Hide resolved
@PatrykSzwer
Copy link
Contributor Author

I included your requested changes and tested this out with the old and new blending app. Please re-review.

@warpech
Copy link
Contributor

warpech commented Apr 26, 2019

There are 7 failing WCT tests with the following message:

Error: Cannot read property 'filter' of undefined
                                 <unknown> at /components/starcounter-include/starcounter-include.html:346
  HTMLElement.StarcounterIncludePrototype._renderCompositionChange at /components/starcounter-include/starcounter-include.html:274
  HTMLElement.StarcounterIncludePrototype.updateComposition at /components/starcounter-include/starcounter-include.html:342
                           HTMLElement.set at /components/starcounter-include/starcounter-include.html:105
                       Context.<anonymous> at cleanup_removed_partial_Layout.html:41

To reproduce locally, follow the instruction from https://github.com/Starcounter/starcounter-include/blob/master/CONTRIBUTING.md#developing-the-element

The problem comes from the line that begins with const defaultViewHtmlsToBeRendered = compositionProvider.ActualViewUris.filter. It comes from the fact that we removed this if: https://github.com/Starcounter/starcounter-include/blob/master/starcounter-include.html#L315. Some tests only check the legacy JSON structure and do not contain ViewUris nor ActualViewUris. Can you please add ViewUris nor ActualViewUris to the fixtures of the failing tests? Then the tests should pass.

}
const defaultViewHtmlsToBeRendered =
compositionProvider.ActualViewUris.filter(
x => !compositionProvider.ViewUris.includes(x.hasOwnProperty("BlendingUri") ? x.BlendingUri : x))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for the path that contain BlendingUri?

Copy link
Contributor

Choose a reason for hiding this comment

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

Today is @PatrykSzwer's last day, so he wont add anything but I may.

Can you add a test for the path that contain BlendingUri?

This whole section never had a single test. This PR does not make it much worse, but I agree tests should be added.

const defaultViewHtmlsToBeRendered =
compositionProvider.ActualViewUris.filter(
x => !compositionProvider.ViewUris.includes(x.hasOwnProperty("BlendingUri") ? x.BlendingUri : x))
.map(x => x.hasOwnProperty("ViewHtml") ? x.ViewHtml : x);
const keys = Object.keys(this.viewModel);
for (let key of keys) {
const scoped = this.viewModel[key];
if (scoped && scoped.Html && defaultViewHtmlsToBeRendered.includes(scoped.Html)) {
if (this.defaultComposition) {
this.template.scopedNodes.forEach((nodes) => {
if (nodes.scope === key) {
nodes.forEach((node) => appendComposition(customComposition, node));
}
});
}
else {
// BUG,TODO (tomalec): looks like a bug
// This means that if there *is* a custom composition, but it does not cover all views,
// and it happend that none of the views had a default composition,
// we would discard custom composition and use default -> fallback
return false;
}
}
}

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

3 participants