Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Added activeIndex class to repeater #51

Merged
merged 15 commits into from
Jul 26, 2019

Conversation

Blackbaud-AlexKingman
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman commented Jul 17, 2019

Addresses #43

Style should be similar to vertical tabs:
https://developer.blackbaud.com/skyux/components/vertical-tabs#demo

This class will be used to show which item is active when using in conjunction with split view component.

Docs update needed. Issue filed here: blackbaud/skyux2-docs#575

color: $sky-text-color-default;
padding-left: 6px;
border-left: $sky-nav-selected-border-width solid $sky-highlight-color-info;
}
Copy link
Contributor Author

@Blackbaud-AlexKingman Blackbaud-AlexKingman Jul 17, 2019

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #51 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   99.52%   99.57%   +0.04%     
==========================================
  Files          24       24              
  Lines         424      470      +46     
  Branches       41       46       +5     
==========================================
+ Hits          422      468      +46     
  Misses          2        2
Impacted Files Coverage Δ
...rc/app/public/modules/repeater/repeater.service.ts 100% <100%> (ø) ⬆️
...public/modules/repeater/repeater-item.component.ts 100% <100%> (ø) ⬆️
.../app/public/modules/repeater/repeater.component.ts 100% <100%> (ø) ⬆️

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 ac9f256...c0a408b. Read the comment docs.

Copy link
Contributor

@Blackbaud-ToddRoberts Blackbaud-ToddRoberts left a comment

Choose a reason for hiding this comment

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

styling looks correct to me

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Thanks so much for the detailed comments; helps a ton when reviewing.

I know the visual demo only allows for one active item at a time, but it's possible to make more than one item active if the consumer wishes. I'm thinking we'd like to avoid that kind of behavior and force the active state to only one of the items?

What if you put an activeItemIndex on the <sky-repeater> component instead?

@Blackbaud-AlexKingman Blackbaud-AlexKingman changed the title Added isActive class to repeater Added activeIndex class to repeater Jul 22, 2019
@Blackbaud-AlexKingman
Copy link
Contributor Author

Updated to go off activeIndex property on sky-repeater instead of each individual repeater item. This is based on the same patter tabs is using.

Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

One quick comment.

src/app/public/modules/repeater/repeater.service.ts Outdated Show resolved Hide resolved
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

I think there are a few structural changes that are needed. The repeater items should handle their own registration (and unregistration) with the service in their ngOnInit and ngOnDestroy. If we need to talk it through offline, let me know!

src/app/public/modules/repeater/repeater-item.component.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater.component.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater-item.component.ts Outdated Show resolved Hide resolved
src/app/public/modules/repeater/repeater.component.ts Outdated Show resolved Hide resolved
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Much better. Great work!

@Blackbaud-AlexKingman Blackbaud-AlexKingman merged commit 9709da6 into master Jul 26, 2019
@Blackbaud-AlexKingman Blackbaud-AlexKingman deleted the repeater-is-active-class branch November 4, 2019 16:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants