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

Moved some examples to new /examples route #213

Merged
merged 7 commits into from
Mar 24, 2017

Conversation

damnko
Copy link
Contributor

@damnko damnko commented Mar 9, 2017

Hi, I created a new "Examples" route with some subpages with all the examples divided by context.
This should hopefully solve #184

I wanted to place all the examples in a separate module and load it asynchronously but I kept having problems with Webpack.
These are the loadChildren test codes I used with the angular2-load-children-loader Webpack loader

loadChildren: () => require("./examples/examples.module")("ExamplesModule")
loadChildren: 'es6-promise?,[name]!./examples/examples.module#ExamplesModule'
loadChildren: 'es6-promise!./examples/examples.module#ExamplesModule'

None of these worked and Webpack kept throwing the following error:
Module build failed: TypeError: Can't add property q9ybxdsfw29, object is not extensible

If you have any hint on how to solve this please let me know. Thanks.

<h1 class="project-name"><a routerLink="/">ng2-smart-table</a></h1>
<h2 class="project-tagline">{{ tagline }}</h2>

<a routerLinkActive="active" routerLink="/demo" class="btn">Quick Start</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can write routerLink="demo" and etc. without slash.

<a routerLinkActive="active" routerLink="/documentation" class="btn">Documentation</a>
<a target="_blank" href="https://github.com/akveo/ng2-smart-table" class="btn">View on GitHub</a>
</section>
<header-component [tagline]="'Documentation'"></header-component>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can write a literal value like tagline="Documentation".

@@ -0,0 +1,29 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename file to examples.routes.ts.

@@ -0,0 +1,25 @@
<h2>Customized edit and view cells examples</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, for file naming use angular style guide. For example: custom-edit-view-examples.component.ts, custom-edit-view-examples.component.html, custom-edit-view-examples.component.scss, custom-edit-view-examples.service.ts.

@lexzhukov
Copy link
Contributor

@damnko how to implement async routes you can see in ng2-admin repository. PR looks good 😃 .

@damnko
Copy link
Contributor Author

damnko commented Mar 10, 2017

Hi, I have moved the /examples section to a dedicated async module but I am still having problems when running npm run build:aot, I am getting this error

    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at afterInnerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/Resolver.js:138:10)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at Resolver.applyPluginsAsyncSeriesBailResult1 (/ng2-smart-table/node_modules/tapable/lib/Tapable.js:181:46)
    at innerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/Resolver.js:125:19)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at /ng2-smart-table/node_modules/tapable/lib/Tapable.js:283:15
    at innerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/Resolver.js:123:11)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at /ng2-smart-table/node_modules/tapable/lib/Tapable.js:283:15
    at resolver.doResolve.createInnerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:44:6)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at afterInnerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/Resolver.js:138:10)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at next (/ng2-smart-table/node_modules/tapable/lib/Tapable.js:188:11)
    at innerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/Resolver.js:123:11)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at /ng2-smart-table/node_modules/tapable/lib/Tapable.js:283:15
    at resolver.doResolve.createInnerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/DescriptionFilePlugin.js:44:6)
    at loggingCallbackWrapper (/ng2-smart-table/node_modules/enhanced-resolve/lib/createInnerCallback.js:31:19)
    at afterInnerCallback (/ng2-smart-table/node_modules/enhanced-resolve/lib/Resolver.js:138:10)
 @ ./demo/src/main.browser.aot.ts 11:29-88

When doing npm start everything seems to work correctly.

Could you give it a try and see if you come up with anything that can solve this issue?

I fixed all the suggestions you wrote me on the previous commit apart from changing the routerLink from absolute to relative because by removing the / it appends the url each time (eg. by clicking demo first and then documentation you get /demo/documentation).

Please let me know should you have any question or suggestion

@lexzhukov
Copy link
Contributor

lexzhukov commented Mar 14, 2017

@damnko could you please do a quick fix, replace async routes with components and we will be ready to merge it.

@nnixaa
Copy link
Contributor

nnixaa commented Mar 24, 2017

Hey @damnko, really looking forward to your input on this :)

@lexzhukov lexzhukov merged commit cec58da into akveo:master Mar 24, 2017
@damnko
Copy link
Contributor Author

damnko commented Mar 24, 2017

Hi, I'm sorry for not being responsive.
The fact is my first PR had sync routing, then I moved to the async version and then I was asked to roll back to sync routing again. I preferred to have a feedback on how to solve the problem rather then bypassing it.

@nnixaa
Copy link
Contributor

nnixaa commented Mar 24, 2017

Hey @damnko, thanks for your response. Well, in this very case, we decided that at the moment it's much more important to have the examples pages working properly rather than have the async version of the routes :)
Anyway, we already merged the PR and @lexzhukov will complete the integration, then we can see if it makes sense to proceed with the async version or leave it for now.

Thanks for your work!

@damnko
Copy link
Contributor Author

damnko commented Mar 24, 2017

Sure I understand, thanks for your feedback.

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.

3 participants