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

Add's a slow process to the Kitchen Sink demo #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keneasson
Copy link

For testing the new Augury-labs Red/Green indicator. Uses settimeout recursively to simulate a slow component.

`,
styleUrls: ['./leaky-faucet.component.css']
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't have line breaks here



export class LeakyFaucetComponent {

Copy link
Contributor

Choose a reason for hiding this comment

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

Line breaks after opening braces are not standard and should be avoided (don't adhere to lint / standard rules)

import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';

import { AppModule } from './app/app.module';
import { environment } from './environments/environment';

// @todo: how to not include in prod bundle?
import { auguryBootstrap } from '../../../augury-labs/packages/core/dist'; // '@augury/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of using a relative import like this considering this is a completely different project it's using. It actually feels more like this sort of test might be better placed in augury-labs. At a minimal we should use the new delivery model then @santiago-elustondo and I came up with instead of this old approach.

This requires 3 file changes

main.augury.ts (new)
angular.json (update transform to use this on new build config option)
package.json (optional - add new serve option with new build config option)


status = 'not leaking';
drip = 'start the drip';
start = 'Start';
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable name start doesn't make much sense since it can change to stop. Perhaps this is the state?

<p>{{ status }}</p>
<button type="button" class="btn btn-danger"
(click)="toggleLeak()">
{{ start }} leaking ( calls a recursive function using setTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after ( ?

<div>
<p>{{ status }}</p>
<button type="button" class="btn btn-danger"
(click)="toggleLeak()">
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, I would probably keep this (click) on the previous line. Unless your going to add a new line for every attribute.

@Component({
selector: 'leaky-faucet',
template: `
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent the template content so it's not inline with the attributes:

template: `
  <div>

@keneasson keneasson force-pushed the leaky-faucet branch 2 times, most recently from baab1ee to 887c406 Compare August 17, 2018 18:11
</div>
`
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we don't leave gaps between the component decorator and the class

this.drip = ' fixed ';
this.stopLeak();
},
this.dripRate );
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't line break here. Just start using prettier to format and I'll unlikely have to comment on small stuff like this ;)

@@ -1,14 +1,13 @@
import { enableProdMode } from '@angular/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are using main.augury.ts these changes should probably be reverted.

import { environment } from './environments/environment';

// @todo: how to not include in prod bundle?
import { auguryBootstrap } from '../../../augury-labs/packages/core/dist'; // '@augury/core';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should look at using optional npm packages instead of using relative paths here.

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

3 participants