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

[Focused Improvements] Hidden renderings do not have implementation and result in console error message #834

Merged
merged 21 commits into from
Oct 28, 2021

Conversation

illiakovalenko
Copy link
Contributor

@illiakovalenko illiakovalenko commented Oct 19, 2021

Description / Motivation

[Focused Improvements] Hidden renderings do not have implementation and result in console error message

HiddenRenderings component will be rendered in case if rendering hidden by personalization
Added packages to component factories, it allows us to import multiple components from the single package

Testing Details

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update (non-breaking change; modified files are limited to the /docs directory)

@vercel
Copy link

vercel bot commented Oct 19, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sitecore-jss/edge-e2e-styleguide/6cTR63mVMhS6cDXTMCkGWTdbzGwJ
✅ Preview: https://edge-e2e-styleguide-git-bugfix-471021-sitecore-jss.vercel.app

[Deployment for e0c620e failed]

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple minor things. In general though, I'm wondering if we couldn't have taken a similar approach as we do with Missing Component (injected directly in the framework Placeholder component implementations w/ allowed override if needed)? This approach wouldn't require any change in the samples / component factories.

import { Component } from '@angular/core';
@Component({
selector: 'sc-hidden-rendering',
host: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the decision to use host here instead of template (like missing-component.component.ts)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, we need to style wrapper (host means sc-hidden-rendering tag), in case if I will add template and style it, EE chromes will not be rendered correctly

samples/nextjs/scripts/templates/component-factory.ts Outdated Show resolved Hide resolved
@illiakovalenko
Copy link
Contributor Author

illiakovalenko commented Oct 22, 2021

@ambrauer HiddenRendering component is the same component as others, and missing component rendered if no component is found. In this case it means that we will need to directly check:

if (componentName === 'HiddenRendering') { return <HiddenRendering /> };

Hidden Rendering data doesn't have difference if compare with other components

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Looks good, see couple comments. Also, I noticed the Angular sample didn't get the new "packages" treatment for component factory. Should it?

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.

2 participants