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

[Angular] Allow for canactivate and resolvers #376

Merged
merged 16 commits into from
Nov 22, 2022

Conversation

jlast
Copy link
Contributor

@jlast jlast commented May 19, 2020

With these changes you can add canactive and resolvers to your application, allowing for components to load their data before rendering.

Combine this with the sc-placeholder loaded event for optimal loading experience.

Description

With these changes you can add canactive and resolvers to your application, allowing for components to load their data before rendering.

Combine this with the sc-placeholder loaded event for optimal loading experience.

Motivation

We needed a better loading experience for customers. This change allowed us to add a pretty skeleton loading before the data was loaded.

How Has This Been Tested?

We included this change into the application.

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)

Checklist:

  • I have read the Contributing guide.
  • My code follows the code style of this project.
  • My code/comments/docs fully adhere to the Code of Conduct.
  • My change is a code change and it requires an update to the documentation.
  • My change is a documentation change and it requires an update to the navigation.

@jlast jlast changed the title Allow for canactivate and resolvers [Angular] Allow for canactivate and resolvers May 19, 2020
@sc-illiakovalenko
Copy link
Contributor

@jlast Can you remove changes related to code formatting? Seems that some formatter is installed in your workspace.
It's hard to analyse your changes.

@jlast
Copy link
Contributor Author

jlast commented Jul 8, 2020

@jlast Can you remove changes related to code formatting? Seems that some formatter is installed in your workspace.
It's hard to analyse your changes.

Thanks for the reply, updated the pull request

@sc-illiakovalenko
Copy link
Contributor

@jlast Please, check again, build is failed

@jlast
Copy link
Contributor Author

jlast commented Jul 8, 2020

@sc-illiakovalenko Fixed an import, all checks have passed now.

@sc-illiakovalenko
Copy link
Contributor

sc-illiakovalenko commented Jul 10, 2020

@jlast But you still have a lot of changes related to code formatting In the last commit you added it. Please, revert it - carefully

@jlast
Copy link
Contributor Author

jlast commented Jul 10, 2020

@jlast But you still have a lot of changes related to code formatting In the last commit you added it. Please, revert it - carefully

@sc-illiakovalenko I should keep a dedicated no-format-on-save config for this project 😄 Thanks for your patience, pipelines are succeeding and reformatting is all undone.

@sc-illiakovalenko
Copy link
Contributor

@jlast No problem 😄
Please, revert your formatting changes in, check it
packages/sitecore-jss-angular/src/components/placeholder.component.ts
packages/sitecore-jss-angular/src/components/render-component.component.ts
packages/sitecore-jss-angular/src/jss-component-factory.service.ts

@sc-illiakovalenko
Copy link
Contributor

@jlast For example

image

@jlast
Copy link
Contributor Author

jlast commented Jul 10, 2020

@sc-illiakovalenko alright, updated it

{
path: 'SecondComponent',
loadChildren: import('./my/my.module').then(mod => mod.MyModule),
resolve: (input: GuardInput) => { return of(true) }
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlast Can't use this example. Types are invalid. Please, check it and provide valid example

Copy link
Contributor

Choose a reason for hiding this comment

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

@jlast I have to use something like:

resolve: {
          ContentBlock: {
            resolve: (input: GuardInput) => { return of(true) }
          }
        }

But it's also not working, I expected to see true value in data.

{
path: 'FirstComponent',
loadChildren: import('./my/my.module').then(mod => mod.MyModule),
canActivate: (input: GuardInput) => { return of(true) }
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlast I can't use this example, because types are invalid, can you provide a valid example?

Type '(input: GuardInput) => any' is not assignable to type 'JssCanActivate | Type<JssCanActivate> | (JssCanActivate | Type<JssCanActivate>)[]'.
  Type '(input: GuardInput) => any' is missing the following properties from type '(JssCanActivate | Type<JssCanActivate>)[]'

Copy link
Contributor

Choose a reason for hiding this comment

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

@jlast I have to use something like:

canActivate: {
        canActivate: (input: GuardInput) => { return of(false) }
}

But it's not correctly

export class AppComponentsModule { }
```

Components can also have a for loading using canActivate and canResolve. Classes can be provided as well, they should be derived from JssCanActivate and/or JssResolve.
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlast Remove extra space in: a for. And I'm not sure, but we don't have canResolve inside the package.
So probably, replace it canResolve -> resolve

function _collectResolverInstances(
factory: ComponentFactoryResult
): Array<[string, JssResolve<any>]> {
if (factory.resolve != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jlast Cover in unit-tests case when resolve is not provided.

@sc-illiakovalenko
Copy link
Contributor

@jlast I want to ask you, switch target branch to: release/15.0.0. And pull it

@jlast
Copy link
Contributor Author

jlast commented Aug 13, 2020

I'll update this merge request probably in 1-2 weeks, working on other priorities for our client at the moment.

@jlast jlast changed the base branch from dev to master December 10, 2020 16:14
@jlast jlast changed the base branch from master to dev December 10, 2020 16:15
@illiakovalenko
Copy link
Contributor

@jlast Thank you for contribution, I pulled your changes in #1246, added some extra stuff and merged

@spike1292 spike1292 deleted the resolver branch December 21, 2023 15:07
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

4 participants