Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

RC > Replaced error component with iframe #436

Merged
merged 5 commits into from
Aug 2, 2018

Conversation

Blackbaud-SteveBrush
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented Jul 23, 2018

Codecov Report

Merging #436 into rc-2.0.0 will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##           rc-2.0.0     #436      +/-   ##
============================================
+ Coverage     99.31%   99.32%   +<.01%     
============================================
  Files            73       73              
  Lines          1910     1912       +2     
  Branches        297      297              
============================================
+ Hits           1897     1899       +2     
  Misses           13       13
Flag Coverage Δ
#builder 100% <100%> (ø) ⬆️
#runtime 95.56% <ø> (ø) ⬆️
#srcapp 100% <ø> (ø) ⬆️
Impacted Files Coverage Δ
lib/sky-pages-route-generator.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c928e6...4363a7f. Read the comment docs.

@Blackbaud-BobbyEarl
Copy link

I know I was absent while you and @Blackbaud-PaulCrowder talked about this, but I don't think I'm a fan of this approach as it currently stands.

I like the efficiency of referencing our own errors SPA, but I see that as problematic for external consumers and for users who want to customize it. I think I would probably be okay with it, if we gave a better escape hatch.

We do have some logic baked in to not include the ** route if a NotFoundComponent is found, unfortunately, it's not really usable unless you're in "advanced" mode where you define your own routes.

Looking at:

if (skyAppConfig.runtime.handle404) {
const err = `<sky-error errorType="notfound"></sky-error>`;
entities.push({
componentName: 'NotFoundComponent',
componentDefinition: `@Component({ template: '${err}' }) export class NotFoundComponent { }`,
routePath: ['**'],
routeParams: []
});
}

What if we changed that to still register the route but reference the NotFoundComponent that the SPA owner has created, that would be sufficient?

@Blackbaud-BobbyEarl
Copy link

Blackbaud-BobbyEarl commented Aug 1, 2018

Maybe something like the following:

  let notFoundComponent = {
    componentName: 'NotFoundComponent',
    routePath: ['**'],
    routeParams: []
  }

  if (skyAppConfig.runtime.handle404) {
    const err = [
      '<iframe src="https://host.nxt.blackbaud.com/errors/notfound"',
      'style="border:0;height:100vh;width:100%;"',
      `[title]="'builder_page_not_found_iframe_title' | skyAppResources"></iframe>`
    ].join(' ');
    notFoundComponent['componentDefinition'] =
      `@Component({ template: \`${err}\` }) export class NotFoundComponent { }`;
  }

  entities.push(notFoundComponent);

To replace

if (skyAppConfig.runtime.handle404) {
const err = `<sky-error errorType="notfound"></sky-error>`;
entities.push({
componentName: 'NotFoundComponent',
componentDefinition: `@Component({ template: '${err}' }) export class NotFoundComponent { }`,
routePath: ['**'],
routeParams: []
});
}

@Blackbaud-BobbyEarl
Copy link

@blackbaud-johnly we should probably take this opportunity to document this functionality, which I was about to do, but I'm having a hard time deciding a good place for it. Happen to have any suggestions?

@Blackbaud-SteveBrush Blackbaud-SteveBrush merged commit 85c47c2 into rc-2.0.0 Aug 2, 2018
@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-BobbyEarl @blackbaud-johnly Think we should start a rc- branch on the docs repo?

@Blackbaud-SteveBrush Blackbaud-SteveBrush deleted the rc-404-iframe branch August 2, 2018 14:26
Blackbaud-DiHuynh pushed a commit to Blackbaud-DiHuynh/skyux-builder that referenced this pull request Jul 10, 2020
* RC > Removed all references to SKY UX, changed dependency structure (blackbaud#419)

* RC > Updated TSLint rules (blackbaud#422)

* Removed legacy omnibar (blackbaud#420)

* RC > Config params as an object; always decode URL params (blackbaud#421)

* RC > Always provide `envId` regardless of permission scope (blackbaud#427)

* RC > Allow SPAs to bundle stylesheets that live outside of `src/app` (blackbaud#428)

* RC > Adjusted dev dependencies (blackbaud#429)

* RC > Fixed ts-helpers for build (blackbaud#434)

* RC > Update from master (blackbaud#425)

* RC > Removed global RxJS imports (blackbaud#438)

* RC > Replaced error component with iframe (blackbaud#436)

* RC > Removed SKY CSS import (blackbaud#443)

* RC > Instrument different directory for libraries (blackbaud#448)

* RC > Do not ignore Protractor Error 199 (blackbaud#435)

* RC > Merged master (blackbaud#444)

* RC > Merge master (blackbaud#454)

* RC > Upgrade Angular, RxJS, TypeScript (blackbaud#495)

* RC > Moved auth-client to peer dependency; fixed `skyux watch` (blackbaud#503)

* Disabled webpack host check (blackbaud#505)

* Replaced JSHint with ESLint. (blackbaud#506)

* RC > Merge master (blackbaud#508)

* RC > Changed name of NPM package (blackbaud#501)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants