feat(stark-ui): example Viewer + basic showcase layout #480
Conversation
|
Should be okay with the missing show as script added back! LGTM! |
There was a problem hiding this comment.
Just some small changes.
Apart from that, the build in Travis failed due to this issue:
"ERROR in : Couldn't resolve resource button-types-example.css relative to /home/travis/build/NationalBankBelgium/stark/showcase/src/assets/examples/test.ts"
You can check it here: https://travis-ci.org/NationalBankBelgium/stark/jobs/399592862
showcase/src/app/app.routes.ts
Outdated
| { name: "index", url: "/", component: HomeComponent }, | ||
| { name: "home", url: "/home", component: HomeComponent }, | ||
| { name: "home", url: "/", component: HomeComponent }, | ||
| { name: "demo-exemple-viewer", url: "/demo-exemple-viewer", component: DemoComponent }, |
There was a problem hiding this comment.
Typo: "exemple" => "example"
| export class ExampleViewerService { | ||
| public constructor(private http: HttpClient) {} | ||
|
|
||
| public fetchFileAlt(path: string): Observable<string> { |
There was a problem hiding this comment.
Question... Why does the name of this method ends with "Alt" ? Perhaps is simpler to call it fetchFile() only.
| import { Observable } from "rxjs"; | ||
|
|
||
| @Injectable() | ||
| export class ExampleViewerService { |
There was a problem hiding this comment.
Maybe we can rename this service to something more generic that really describes what it does: fetching files.
I know this service is only used by the example viewer component for now but maybe we can use it later for other components/features.
There was a problem hiding this comment.
True, maybe we can call it FileService, so we could extend it easier if we want to add other methods linked to file manipulation
| }); | ||
| }); | ||
|
|
||
| describe("example-viewer-service", () => { |
There was a problem hiding this comment.
This test suite ("describe" block) does not belong here. This spec.ts file are only related to the example viewer component.
The tests related to the service should be in a different spec.ts file ;)
| }); | ||
| }); | ||
|
|
||
| describe("@Input() extensions", () => { |
There was a problem hiding this comment.
Let's put all the describe blocks related to inputs at the top and the rest of tests after so we can have more or less a convention on how to structure our unit tests ;)
| export function highlightJsFactory(): any { | ||
| return hljs; | ||
| } | ||
| fdescribe("ExampleViewerComponent", () => { |
There was a problem hiding this comment.
Don't forget to remove the "f" before the "describe", otherwise only this test will be executed in Travis!!
| MatTabsModule, | ||
| CommonModule | ||
| ], | ||
| providers: [], |
There was a problem hiding this comment.
Question, isn't the service to fetch the files supposed to be included in here? I think it has not been defined in any module right? or am I missing something?
There was a problem hiding this comment.
Doesn t make any difference for now but true it is more correct to provide it
| .fetchFile("/assets/examples/" + this.filesPath + "." + extension.toLowerCase()) | ||
| .subscribe( | ||
| (data: string) => this.addFileContent({ extension: extension, file: data }), | ||
| (error: HttpErrorResponse) => console.log(error) |
There was a problem hiding this comment.
One small change: just replace this console.log by the StarkLogging service
Normally we should always use the StarkLogging service. We should use console In case it is not possible to use the Stark service or it is really necessary.
There was a problem hiding this comment.
Finally we'll leave the console in there because there is an issue with the Stark mocks. Tracked in #484
ISSUES CLOSED: #458, #459
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #458, #459