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

Stdev dcmorales t78 make a loader #79

Closed
wants to merge 13 commits into from

Conversation

stdevNorge
Copy link
Contributor

No description provided.

.gitignore Outdated
@@ -42,3 +42,8 @@ Thumbs.db

# compiled binaries
/explorer.*
/dist/vendor.181584baefd2a47c5eda.bundle.js
Copy link
Contributor

Choose a reason for hiding this comment

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

@stdevDcmorales you should add a generic rule here e.g. dist/* or dist/*.bundle.* .

.gitignore Outdated
/dist/skycoin.ico
/dist/styles.ad5f424b95bd73aa89ae.bundle.css
/dist/vendor.1c15142c9c6efd45619a.bundle.js
/node_modules/angular-font-awesome
Copy link
Contributor

Choose a reason for hiding this comment

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

Same again .. just ignore node_modules/* as a whole .

@gz-c
Copy link
Member

gz-c commented Jan 7, 2018

We commit dist to the repo so that we dont have to install nodejs to deploy

@@ -21,6 +21,7 @@
"prefix": "app",
"styles": [
"assets/bootstrap.min.css",
"../node_modules/font-awesome/css/font-awesome.css",
Copy link
Member

Choose a reason for hiding this comment

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

doesnt this need to be in assets?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, copy to assets and use a minimized lib

@samuelvisscher
Copy link
Contributor

Sorry for possible conflicting messages, but please leave old dist in place without generating new dist. Dist files should be compiled by core members (steve, me, iketheadore, synth)

@olemis
Copy link
Contributor

olemis commented Jan 15, 2018

@stdevDcmorales I see some merge conflicts in this branch .

@@ -21,6 +21,7 @@
"prefix": "app",
"styles": [
"assets/bootstrap.min.css",
"../node_modules/font-awesome/css/font-awesome.css",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, copy to assets and use a minimized lib

@@ -42,3 +42,4 @@ Thumbs.db

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 add dist to gitignore

Copy link
Contributor

Choose a reason for hiding this comment

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

To take away npm dependency on our servers

@@ -1,92 +0,0 @@
core-js@2.5.1
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 rebuild dist files, Steve or I will generate dist files. This makes merging, reviewing and ensuring code integrity easier.

@@ -22,7 +22,9 @@
"@angular/platform-browser": "^4.0.0",
"@angular/platform-browser-dynamic": "^4.0.0",
"@angular/router": "^4.0.0",
"angular-font-awesome": "^3.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use an angular package for something as trivial as fa

"core-js": "^2.4.1",
"font-awesome": "^4.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

just download the min.css

import { BlockDetailsComponent } from './components/pages/block-details/block-details.component';
import { TransactionDetailComponent } from './components/pages/transaction-detail/transaction-detail.component';
import { AddressDetailComponent } from './components/pages/address-detail/address-detail.component';
import { TransactionsValuePipe } from './pipes/transactions-value.pipe';
import { ExplorerService } from './services/explorer/explorer.service';
import { QrCodeComponent } from './components/layout/qr-code/qr-code.component';
import { FormsModule } from '@angular/forms';
import { SharedModule } from "app/modules/shared/shared.module";

Copy link
Contributor

Choose a reason for hiding this comment

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

Although using a SharedModule is quite common, due to the size of the application we have chosen not to do so

let component: BlockChainTableComponent;
let fixture: ComponentFixture<BlockChainTableComponent>;
describe('BlocksComponent', () => {
let component: BlocksComponent;
Copy link
Contributor

Choose a reason for hiding this comment

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

We use 2 spaces for indentation


beforeEach(async(() => {
TestBed.configureTestingModule({
declarations: [ BlockChainTableComponent ]
declarations: [BlocksComponent ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please maintain spaces between square brackets

})
export class LoadingComponent implements OnInit {

@Input() name: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really overengineered for what we're doing

@@ -11,6 +11,10 @@
<link rel="icon" type="image/x-icon" href="skycoin.ico">
</head>
<body>
<app-root></app-root>
<app-root>
Copy link
Contributor

Choose a reason for hiding this comment

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

The spinner was intended for inside the table

Copy link
Contributor

Choose a reason for hiding this comment

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

And on loading a new address

@samuelvisscher
Copy link
Contributor

Will include this functionality in my next PR, so closing

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

6 participants