Added SkyLinkRecordsComponent #1036

blackbaud-joshgerdes · 2017-08-31T17:51:53Z

Contributing record linking component as defined in #726

blackbaud-sky-savage · 2017-08-31T18:19:56Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 5847fae
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/270521037

(Please note that this is a fully automated comment.)

codecov-io · 2017-08-31T18:21:02Z

Codecov Report

Merging #1036 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #1036    +/-   ##
=======================================
  Coverage     100%    100%            
=======================================
  Files         317     351    +34     
  Lines        5983    6477   +494     
  Branches      760     838    +78     
=======================================
+ Hits         5983    6477   +494

Impacted Files	Coverage Δ
...link-records/state/results/results.orchestrator.ts	`100% <100%> (ø)`
...link-records/state/selected/set-selected.action.ts	`100% <100%> (ø)`
...s/link-records/state/link-records-state.rxstate.ts	`100% <100%> (ø)`
.../link-records/link-records-item-title.component.ts	`100% <100%> (ø)`
src/modules/link-records/state/fields/actions.ts	`100% <100%> (ø)`
src/modules/list/list.component.ts	`100% <100%> (ø)`	⬆️
...c/modules/link-records/state/fields/field.model.ts	`100% <100%> (ø)`
...s/link-records/state/fields/fields.orchestrator.ts	`100% <100%> (ø)`
...es/link-records/state/matches/set-status.action.ts	`100% <100%> (ø)`
src/modules/link-records/link-records.component.ts	`100% <100%> (ø)`
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2978f7...7ec0f85. Read the comment docs.

blackbaud-sky-savage · 2017-08-31T20:35:43Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: e7242ac
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/270565886

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-05T15:59:27Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: b28e226
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/272105282

(Please note that this is a fully automated comment.)

Blackbaud-ToddRoberts

To ensure consistency across components styling should reference mixins and variables to the extent possible.
Text styling should reference the typography mixins or classes.
Borders should reference the border mixin https://github.com/blackbaud/skyux-design-tokens/blob/master/src/scss/mixins.scss
Colors should reference the variables documented here https://developer.blackbaud.com/skyux2/design/color rather than referencing the numeric values, e.g. $sky-highlight-color-success instead of $sky-color-green-50

blackbaud-joshgerdes · 2017-09-06T14:21:58Z

@Blackbaud-ToddRoberts thanks for the review. The design updates were not complete when we originally created this control. And the variables were missing from the skyux-design-tokens repo so I was unable to update our contrib repo where this control currently resides. I did a quick pass when creating this PR but I will run through it again to update some more of the styles.

In general, is the skyux-design-tokens repo stable/up-to-date? It seems to be missing the variables.scss file and the mixins.scss has only one mixin in it? We would like to update our contrib repo with that latest stylings at some point (without having to copy over scss on every skyux update) and was hoping to leverage the scss in that repo. What are the skyux team's overall goals for that repo?

Blackbaud-ToddRoberts · 2017-09-06T14:44:48Z

@Blackbaud-BobbyEarl can you answer the question about the design-tokens repo? I am not sure where the variables are at.

blackbaud-joshgerdes · 2017-09-06T14:49:17Z

@Blackbaud-ToddRoberts Updated. For the typography, we are currently using the new font families but the we would need direction from UX ( @Blackbaud-MattVaccaro ) to determine what classes they would want to switch/update the control elements to. Right now the typography has already been approved by him.

blackbaud-sky-savage · 2017-09-06T15:13:44Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 7299d13
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/272516971

(Please note that this is a fully automated comment.)

Blackbaud-ToddRoberts · 2017-09-06T15:22:21Z

@blackbaud-joshgerdes The ultimate intent of the design tokens repo is to provide a single source of truth for core styling elements, consumable by products regardless of platform. In general we want styling to be handled automatically by components and SkyUX-provided CSS classes, but we know there are some cases where that isn't optimal or feasible. The design token variable and mixin files provide the styles we want to expose for consumption in apps in addition to the SkyUX CSS classes; we will likely add to these in the future. When building a component, the SkyUX mixin and variables files can be used as well. @Blackbaud-PaulCrowder please correct any misstatements in there ;)

Blackbaud-BobbyEarl · 2017-09-06T18:01:34Z

The skyux-design-tokens repo is the source. Many of the assets, such as variables.scss are created during the build step before it's published to NPM. If you look at a consuming project, say skyux2, you'll see all the necessary files available for reference.

blackbaud-sky-savage · 2017-09-07T02:39:36Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: b4396a3
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/272732449

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-07T13:52:30Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 3a4ab76
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/272894152

(Please note that this is a fully automated comment.)

blackbaud-joshgerdes · 2017-09-07T14:03:35Z

@Blackbaud-ToddRoberts did you get a chance to review the updates. Anything else you need me to update on this PR?

Blackbaud-ToddRoberts · 2017-09-07T20:16:15Z

@blackbaud-joshgerdes I am looking at it now, in the demo it looks like some styles are being overwritten by Stache styles so I'm trying to ensure it looks right outside of a Stache environment.

blackbaud-sky-savage · 2017-09-07T23:22:00Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 1ae8572
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/273096820

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-08T13:59:32Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: b4f1a37
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/273295131

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-11T18:55:37Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 5d32d1d
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/274292476

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-18T15:20:06Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: d656abe
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/276875526

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-18T17:42:23Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 9986ae9
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/276942272

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-18T18:22:05Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 249b341
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/276954220

(Please note that this is a fully automated comment.)

Blackbaud-SteveBrush · 2017-09-18T20:15:00Z

@blackbaud-joshgerdes Here is my initial feedback. Thanks for putting this together!

Provide a prefix for the STATUSES variable (for example, SKY_LINK_RECORDS_STATUSES), since it could potentially be imported in another module. If the intention is that it should not be used by another module, you could remove the export in the index.ts file.
Add entries for any English labels or descriptions to the resources_en_US.json file.
The HTML template for link-records-item.component.html file is fairly complex and the logic is difficult to read. Is there any possible way we could replace some (if not all) of the *ngIf statements with calls to component methods? Ideally, the STATUSES wouldn't be exposed in the template, and the logic would be contained in the class. For example:

<div *ngIf="record.status == STATUSES.Suggested || record.status == STATUSES.Selected">

// Convert to...

<div *ngIf="isPossibleMatch(record)"> // (or something similar)

Blackbaud-SteveBrush · 2017-09-18T20:20:56Z

src/modules/link-records/link-records.component.ts

+  ) {}
+
+  public ngOnInit() {
+    if (this.items && !(this.items instanceof Observable)) {


For these if blocks: what's the use case for checking if the input is of type observable? It seems that we should enforce one type or the other (ie. the consumer should always provide an observable, here.). Just curious if you have any insights...

It allows the user to provide an observable or regular array for the values. This is how we implemented similar properties when we created the sky list and its views as well.

As for the above:

Will update to SKY_LINK_RECORDS_STATUSES

Not explicitly part of the contribution guidelines but we will update

I actually think the logic is more readable because it is in place in the file. Most of the if logic is just around a single state anyway. I could see moving a couple if statements to functions but don't really see a huge gain by doing so. The logic for the link record items just inherently has some complexity to it.

I'll add an issue to update the contribution guidelines, based on your feedback for #2. (#1082)

Sounds good, made the other requested updates. Tests passed and just awaiting review.

blackbaud-sky-savage · 2017-09-19T14:46:31Z

Tests failed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: d8dc590
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/277338093

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-19T15:27:17Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 2737d3c
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/277350458

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-19T15:56:56Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 69c58e1
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/277359565

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-19T18:09:30Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 6eb9c43
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/277417872

(Please note that this is a fully automated comment.)

blackbaud-sky-savage · 2017-09-19T20:48:47Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: f276afb
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/277478348

(Please note that this is a fully automated comment.)

Blackbaud-SteveBrush · 2017-09-19T21:21:00Z

@blackbaud-joshgerdes Thanks for taking care of those updates! Most of this looks good to me.

Something I noticed: some of the TypeScript classes do not have a prefix of Sky. I realize that some of the already-approved List stuff do not have this prefix (for example: https://github.com/blackbaud/skyux2/blob/master/src/modules/list/index.ts), and those should have been caught before being merged. We can't fix those, but it would be ideal if all new components, directives, and providers had the Sky prefix.

Some examples I saw in this branch: LinkRecordsApi, LinkRecordsItemModel, LinkRecordsState, LinkRecordsStateDispatcher, etc.

Thank you!

blackbaud-joshgerdes · 2017-09-19T21:54:54Z

Those are all internal to the component and should not be directly referenced. That is why they do not have the prefix. Do you really still need them to have it?

@Blackbaud-SteveBrush

Blackbaud-SteveBrush · 2017-09-20T17:49:12Z

@blackbaud-joshgerdes Yes, please.

The idea we're shooting for is that any contributor working with our classes would know at first-glance that they are owned by SKY, and not a third-party library. Also, it prevents class-name clashes with other libraries, if it ever came to that. (Let me know if you want some assistance with this, as I know it's no small task.)

It's also regrettable that this requirement is not described in the Contribution Guidelines; I've created an issue to amend this for the future: #1093.

including locale resources and statuses update.

blackbaud-sky-savage · 2017-09-20T22:13:20Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 4b8167a
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/277962624

(Please note that this is a fully automated comment.)

blackbaud-joshgerdes · 2017-09-21T13:50:51Z

@Blackbaud-SteveBrush Updated

blackbaud-sky-savage · 2017-09-21T17:51:58Z

Tests passed. Automated cross-browser testing via BrowserStack and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 7ec0f85
Build details: https://travis-ci.org/blackbaud-sky-savage/skyux2/builds/278281585

(Please note that this is a fully automated comment.)

Blackbaud-SteveBrush

LGTM

blackbaud-joshgerdes mentioned this pull request Aug 31, 2017

Record linking UI #726

Closed

Blackbaud-SteveBrush self-assigned this Aug 31, 2017

Blackbaud-ToddRoberts requested review from Blackbaud-ToddRoberts and Blackbaud-PaulCrowder September 5, 2017 19:29

Blackbaud-ToddRoberts previously requested changes Sep 6, 2017

View reviewed changes

blackbaud-joshgerdes force-pushed the link-records branch from b28e226 to 7299d13 Compare September 6, 2017 14:44

Blackbaud-SteveBrush removed their assignment Sep 6, 2017

blackbaud-joshgerdes force-pushed the link-records branch from e8a55a6 to 1ae8572 Compare September 7, 2017 21:44

blackbaud-joshgerdes force-pushed the link-records branch from 1ae8572 to b4f1a37 Compare September 8, 2017 13:18

Blackbaud-ToddRoberts requested a review from blackbaud-johnly September 8, 2017 17:22

blackbaud-joshgerdes mentioned this pull request Sep 8, 2017

Link records styling #1056

Closed

blackbaud-joshgerdes force-pushed the link-records branch from b4f1a37 to 5d32d1d Compare September 11, 2017 18:24

blackbaud-joshgerdes force-pushed the link-records branch from 5d32d1d to 5f97aa1 Compare September 11, 2017 20:08

blackbaud-joshgerdes force-pushed the link-records branch from d656abe to 9986ae9 Compare September 18, 2017 16:56

Blackbaud-SteveBrush reviewed Sep 18, 2017

View reviewed changes

blackbaud-joshgerdes force-pushed the link-records branch from 249b341 to d8dc590 Compare September 19, 2017 14:31

blackbaud-joshgerdes force-pushed the link-records branch 2 times, most recently from 2737d3c to 69c58e1 Compare September 19, 2017 15:16

blackbaud-joshgerdes force-pushed the link-records branch from 69c58e1 to 6eb9c43 Compare September 19, 2017 17:38

blackbaud-joshgerdes force-pushed the link-records branch from 6eb9c43 to f276afb Compare September 19, 2017 20:19

blackbaud-joshgerdes added 3 commits September 20, 2017 16:52

Added link-records component

bad23c9

Updated link-records per request

18fa4fe

including locale resources and statuses update.

Add ‘Sky’ prefix to internal objects (models, states, etc.) per request

4b8167a

blackbaud-joshgerdes force-pushed the link-records branch from f276afb to 4b8167a Compare September 20, 2017 21:52

Merge branch 'master' into link-records

7ec0f85

Blackbaud-SteveBrush approved these changes Sep 21, 2017

View reviewed changes

Blackbaud-SteveBrush merged commit b7de5e2 into blackbaud:master Sep 21, 2017

blackbaud-joshgerdes deleted the link-records branch October 6, 2017 20:01

Added SkyLinkRecordsComponent #1036

Added SkyLinkRecordsComponent #1036

Conversation

blackbaud-joshgerdes commented Aug 31, 2017

blackbaud-sky-savage commented Aug 31, 2017

codecov-io commented Aug 31, 2017 • edited Loading

Codecov Report

blackbaud-sky-savage commented Aug 31, 2017

blackbaud-sky-savage commented Sep 5, 2017

Blackbaud-ToddRoberts left a comment

Choose a reason for hiding this comment

blackbaud-joshgerdes commented Sep 6, 2017

Blackbaud-ToddRoberts commented Sep 6, 2017 • edited Loading

blackbaud-joshgerdes commented Sep 6, 2017

blackbaud-sky-savage commented Sep 6, 2017

Blackbaud-ToddRoberts commented Sep 6, 2017 • edited Loading

Blackbaud-BobbyEarl commented Sep 6, 2017

blackbaud-sky-savage commented Sep 7, 2017

blackbaud-sky-savage commented Sep 7, 2017

blackbaud-joshgerdes commented Sep 7, 2017

Blackbaud-ToddRoberts commented Sep 7, 2017

blackbaud-sky-savage commented Sep 7, 2017

blackbaud-sky-savage commented Sep 8, 2017

blackbaud-sky-savage commented Sep 11, 2017

blackbaud-sky-savage commented Sep 18, 2017

blackbaud-sky-savage commented Sep 18, 2017

blackbaud-sky-savage commented Sep 18, 2017

Blackbaud-SteveBrush commented Sep 18, 2017

Blackbaud-SteveBrush Sep 18, 2017

Choose a reason for hiding this comment

blackbaud-joshgerdes Sep 18, 2017

Choose a reason for hiding this comment

blackbaud-joshgerdes Sep 18, 2017

Choose a reason for hiding this comment

Blackbaud-SteveBrush Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

blackbaud-joshgerdes Sep 19, 2017

Choose a reason for hiding this comment

blackbaud-sky-savage commented Sep 19, 2017

blackbaud-sky-savage commented Sep 19, 2017

blackbaud-sky-savage commented Sep 19, 2017

blackbaud-sky-savage commented Sep 19, 2017

blackbaud-sky-savage commented Sep 19, 2017

Blackbaud-SteveBrush commented Sep 19, 2017

blackbaud-joshgerdes commented Sep 19, 2017 • edited Loading

Blackbaud-SteveBrush commented Sep 20, 2017

blackbaud-sky-savage commented Sep 20, 2017

blackbaud-joshgerdes commented Sep 21, 2017

blackbaud-sky-savage commented Sep 21, 2017

Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 31, 2017 •

edited

Loading

Blackbaud-ToddRoberts commented Sep 6, 2017 •

edited

Loading

Blackbaud-ToddRoberts commented Sep 6, 2017 •

edited

Loading

Blackbaud-SteveBrush Sep 19, 2017 •

edited

Loading

blackbaud-joshgerdes commented Sep 19, 2017 •

edited

Loading