714 list view grid highlighting #1944
Conversation
|
I think it should be on by default, it's the intended/expected behavior. |
Codecov Report
@@ Coverage Diff @@
## master #1944 +/- ##
========================================
Coverage ? 100%
========================================
Files ? 424
Lines ? 8954
Branches ? 1322
========================================
Hits ? 8954
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
| @@ -1,7 +1,7 @@ | |||
| import { SkyVisualTest } from '../../../config/utils/visual-test-commands'; | |||
| import { element, by } from 'protractor'; | |||
|
|
|||
| describe('grid component', () => { | |||
| fdescribe('grid component', () => { | |||
There was a problem hiding this comment.
Might wanna change this :)
There was a problem hiding this comment.
Facepalm....... Fixed
|
Created #1950 to document the new inputs. ... Please let me know if anything else needs to be documented for this. |
| component.grid.currentSearchText.subscribe(currentText => { | ||
| expect(currentText).toBe('searchText'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Could you add a done() call or something to ensure the expect is always being hit?
There was a problem hiding this comment.
You can't mix fakeAsync and done as done makes the test async. I did some manual testing and see that it is getting hit and based the test off others in this spec. I don't mind changing it to async but know that in another PR I was asked to look into making a test fakeAsync instead of async.
Thoughts @blackbaud-conorwright?
There was a problem hiding this comment.
Yeah, done will not be usable here. As long is this catches when the logic is removed it's fine :)
There was a problem hiding this comment.
I double checked and if the logic to updated currentSearchText is removed then the test fails
| </sky-grid> | ||
| </div> | ||
| <button id="highlight-button" class="sky-btn sky-btn-primary" (click)="triggerHighlight()">Trigger Highlight</button> |
There was a problem hiding this comment.
split attributes out to multiple lines #1647 (comment)
| @@ -8,6 +8,7 @@ | |||
| [selectedColumnIds]="selectedColumnIds | async" | |||
| [columns]="columns | async" | |||
| [hasToolbar]="hasToolbar | async" | |||
| [highlightText]="highlightSearchText ? (currentSearchText | async) : undefined" | |||
There was a problem hiding this comment.
Can highlightSearchText be a getter so that this logic could be handled in the ts file and included in coverage?
There was a problem hiding this comment.
Nevermind. The logic will be much worse, since currentSearchText is an observable. We will leave this as it is
| .takeUntil(this.ngUnsubscribe) | ||
| .subscribe(searchText => { | ||
| this.currentSearchText.next(searchText); | ||
| }); |
There was a problem hiding this comment.
Just a minor suggestion, but can this be reduced?
Like:
public currentSearchText: Observable<string>;
this.currentSearchText = this.state.map(s => s.searchText);
I'm also curious if currentSearchText must be an observable
There was a problem hiding this comment.
Take a look at the reduction, but it should just be an observable considering how the list is already designed.
Blackbaud-AlexKingman
assigned Blackbaud-TrevorBurch and unassigned Blackbaud-AlexKingman
Blackbaud-TrevorBurch
assigned blackbaud-conorwright and unassigned Blackbaud-TrevorBurch
Blackbaud-TrevorBurch
added
Status: Ready to merge
and removed
Status: Ready for review
labels
Resolves #714
@Blackbaud-SteveBrush @Blackbaud-ToddRoberts - I'd like your thoughts on if this should be on by default. Right now I have it on by default as that is what Sky UX 1 is doing. However, I'm also leary about that due to the list view grid/grid components having been in 2 for so long without the highlighting. Should we make it be off by default for now and switch the default back in 3?