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

build refactor #791

Closed
wants to merge 56 commits into
base: staging
from

Conversation

Projects
None yet
2 participants
@RCopeland
Member

RCopeland commented Nov 7, 2018

No description provided.

RCopeland added some commits Nov 7, 2018

@RCopeland RCopeland closed this Nov 8, 2018

@RCopeland RCopeland reopened this Nov 8, 2018

RCopeland added some commits Nov 8, 2018

Merge remote-tracking branch 'upstream/staging' into build-updates
# Conflicts:
#	src/angular/projects/spark-core-angular/src/lib/components/sprk-modal/sprk-modal.component.ts

@RCopeland RCopeland closed this Nov 9, 2018

@RCopeland RCopeland reopened this Nov 9, 2018

@RCopeland RCopeland closed this Nov 9, 2018

@RCopeland RCopeland reopened this Nov 9, 2018

RCopeland added some commits Nov 12, 2018

Merge remote-tracking branch 'upstream/staging' into build-updates
# Conflicts:
#	packages/spark-core/package-lock.json
#	packages/spark-extras-card/package.json
#	packages/spark-extras-dictionary/package.json
#	packages/spark-extras-highlight-board/package-lock.json
#	packages/spark-extras-highlight-board/package.json
#	packages/spark-extras/package.json
#	src/angular/projects/spark-core-angular/package-lock.json
#	src/angular/projects/spark-core-angular/src/lib/components/sprk-footer/sprk-footer.component.ts
#	src/angular/projects/spark-core-angular/src/lib/components/sprk-toggle/sprk-toggle.component.ts
#	src/angular/projects/spark-extras-angular-award/package-lock.json
#	src/angular/projects/spark-extras-angular-card/package-lock.json
#	src/angular/projects/spark-extras-angular-card/package.json
#	src/angular/projects/spark-extras-angular-dictionary/package-lock.json
#	src/angular/projects/spark-extras-angular-highlight-board/package-lock.json
#	src/angular/src/app/spark-docs/footer-docs/footer-docs.component.ts
@afebbraro

I'd like to talk about the new formatting this is implementing. In our current code standard we do not put lone > on their own line. This Build PR seems to also be changing that standard. I realize it's probably a linter that did this and not you doing it intentionally haha. I think we need to configure that linter to match our code style and revert all the > on their own lines.

const spawn = require('child_process').spawn;
const clean = require('gulp-clean');
gulp.task('build-angular-dev-app', (cb) => {

This comment has been minimized.

@afebbraro

afebbraro Nov 13, 2018

Member

If we wanted to reduce how long this is, we could change angular to just ng.

This comment has been minimized.

@RCopeland

RCopeland Nov 13, 2018

Member

i can change them to ng

This comment has been minimized.

@RCopeland

RCopeland Nov 13, 2018

Member

actually, i think i'd rather keep angular, because some of the time the command refers to the directory and i dont want to change its name. I want to keep the use of angular / ng consistant.

@@ -13,12 +13,13 @@
"url": "https://github.com/sparkdesignsystem/spark-design-system/issues"
},
"engines": {
"node": ">=4.0.0"
"node": "8.10.0"

This comment has been minimized.

@afebbraro

afebbraro Nov 13, 2018

Member

I forgot, what was the reason we can't use the latest node version?

This comment has been minimized.

@RCopeland

RCopeland Nov 13, 2018

Member

we decided to use LTS for stability's sake

[attr.data-analytics]="analyticsString"
[attr.data-id]="idString"
(click)="toggleAccordion($event)"
>

This comment has been minimized.

@afebbraro

afebbraro Nov 13, 2018

Member

This formatting is off, with the lone end bracket on one line.

This comment has been minimized.

@RCopeland

RCopeland Nov 13, 2018

Member

i think thats my editor doing it, i can bring it up to the previous line.

@@ -6,7 +6,8 @@ import { Component, Input } from '@angular/core';
<li
[ngClass]="getClasses()"
[attr.data-analytics]="analyticsString"
[attr.data-id]="idString">
[attr.data-id]="idString"
>

This comment has been minimized.

@afebbraro

afebbraro Nov 13, 2018

Member

Hmm, wonder why there's all these lone brackets? Your Editor? Don't think we want to merge that in.

This comment has been minimized.

@RCopeland

RCopeland Nov 13, 2018

Member

same here

@@ -61,7 +61,7 @@ describe('SparkModalComponent', () => {
component.title = 'This is my title';
fixture.detectChanges();
expect(
modalElement.querySelector('.sprk-c-Modal__heading').textContent
modalElement.querySelector('.sprk-c-Modal__heading').textContent.trim()

This comment has been minimized.

@afebbraro

afebbraro Nov 13, 2018

Member

Was the .trim() added as part of the build refactor issue or something else?

This comment has been minimized.

@RCopeland

RCopeland Nov 13, 2018

Member

Added to fix a test which failed because of added spaces in the title after the modals were refactored

RCopeland added some commits Nov 13, 2018

@RCopeland RCopeland closed this Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment