Popover Component #1020

Blackbaud-SteveBrush · 2017-08-28T17:11:29Z

Addresses #167

codecov-io · 2017-08-28T18:34:47Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #1020    +/-   ##
=======================================
  Coverage     100%    100%            
=======================================
  Files         313     317     +4     
  Lines        5777    5976   +199     
  Branches      728     759    +31     
=======================================
+ Hits         5777    5976   +199

Impacted Files	Coverage Δ
...dules/vertical-tabset/vertical-tabset.component.ts	`100% <100%> (ø)`	⬆️
src/modules/popover/popover.module.ts	`100% <100%> (ø)`
...vertical-tabset/vertical-tabset-group.component.ts	`100% <100%> (ø)`	⬆️
src/modules/popover/popover-adapter.service.ts	`100% <100%> (ø)`
src/modules/popover/popover.directive.ts	`100% <100%> (ø)`
src/modules/popover/popover.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 bbd739f...5c567b1. Read the comment docs.

… into component-tooltip

Blackbaud-BobbyEarl · 2017-08-28T21:50:31Z

Hey @Blackbaud-SteveBrush... just some initial feedback.

The animation seemed a bit jarring (as compared to SKY UX 1 popover).
Escape key doesn't close popover.
I would suggest adding something that has a higher order z-index to the demo (dropdown or modal), and more importantly to the tests you write. I know that was a pain point for us in the past.
Add a button (or dropdown) to example HTML content to make sure tabbing isn't affected.
Should there be a "close" link or button as in the SKY UX 1 version?

Code-wise, nothing stood out to me, but I didn't look at it in great detail yet.

… into component-tooltip

Blackbaud-ToddRoberts

I am making some style tweaks

Blackbaud-AdamHickey

Looks good. Just a few small things.

Blackbaud-AdamHickey · 2017-09-11T20:10:26Z

skyux-spa-visual-tests/src/app/popover/popover.visual-spec.ts

+import { SkyVisualTest } from '../../../config/utils/visual-test-commands';
+
+import {
+  // browser,


Here is a comment you may have forgotten to delete.

Blackbaud-AdamHickey · 2017-09-11T20:15:38Z

src/app/components/popover/popover-demo.component.scss

@@ -0,0 +1,39 @@
+.popover-demo-container {


All of the other demo components use inline styles. I'm not sure why that was originally, but should we add this scss file as one of the demo files that shows up in the documentation?

At first I didn't include the stylesheet because I didn't think it was relevant to the demo, but I suppose I should include it just to be consistent.

I'm not 100% sure the stylesheet should be included or not. I was just calling it out because it was different from the other documentation components. I might lean towards including it, but I'm not sold either way.

Blackbaud-AdamHickey · 2017-09-11T20:18:57Z

src/core.ts

@@ -84,6 +86,7 @@ import { SkyWaitModule } from './modules/wait';
    SkyNavbarModule,
    SkyPageSummaryModule,
    SkyPagingModule,
+    SkyPopoverModule,


Do you need to export the popover directive down below?

Great catch.

Blackbaud-AdamHickey · 2017-09-11T20:25:23Z

src/modules/popover/popover-placement.ts

@@ -0,0 +1 @@
+export type SkyPopoverPlacement = 'above' | 'below' | 'right' | 'left';


Oh cool I didn't know typescript had discriminated unions.

added rounded corners and standardized font & spacing

Blackbaud-ToddRoberts

Made some style tweaks if you want to double check them.

One potential concern is how to dismiss a popover on mobile if the content fills the screen and there is not enough empty space to tap to close.

updated

Blackbaud-ToddRoberts · 2017-09-11T21:10:13Z

@Blackbaud-SteveBrush I added a variable for border radius, which will need to be applied to some other components. Should I include those other updates here or in a separate PR after this is merged?

… into component-tooltip

Blackbaud-SteveBrush added 4 commits August 24, 2017 19:36

Added basic component

8888a7a

Added click events and all positions

534e5e6

Updated styles

ebfa2c4

Updated implementation

c4de6c0

Blackbaud-SteveBrush added 3 commits August 28, 2017 14:35

Merge branch 'master' into component-tooltip

cd254b1

Added basic docs

09cc0dc

Merge branch 'component-tooltip' of https://github.com/blackbaud/skyux2…

95e5ac2

… into component-tooltip

Blackbaud-SteveBrush requested a review from Blackbaud-BobbyEarl August 28, 2017 19:05

Blackbaud-SteveBrush added 20 commits August 29, 2017 12:40

Escape key hides popovers

f75d4ce

Updated implementation

9291400

Updated implementation

53e9d8e

Factored out element coordinates method

f591c30

Updated implementation

1903f66

Fixed linting errors

316ab35

Update demo-page-property.component.html

455cea0

Update alert.component.scss

7b789bc

Update grid-column.component.ts

7b6bda3

Update list-secondary-actions.spec.ts

52c49ac

Update list-view.component.ts

8a0aecb

Update list.component.spec.ts

17b5c2a

Update tile-dashboard.service.spec.ts

0037cd4

Added spec files

658d035

Merge branch 'component-tooltip' of https://github.com/blackbaud/skyux2…

2c9e039

… into component-tooltip

Merge branch 'master' into component-tooltip

7eccced

Added basic visual tests

7d4d6a6

Merge branch 'master' into component-tooltip

1aaa8f0

Finished visual tests

3e76786

Added unit tests

2b725e0

Blackbaud-SteveBrush added 5 commits September 7, 2017 17:14

Merge branch 'master' into component-tooltip

6e3ca43

100% code coverage; Using OnPush

3de2219

Removed unused items

7a3d160

Cleaned up docs

59d4c86

Removed comments

9a8ba3b

Blackbaud-SteveBrush changed the title ~~[HOLD] Popover Component~~ Popover Component Sep 8, 2017

Blackbaud-SteveBrush added 4 commits September 8, 2017 14:47

Merge branch 'master' into component-tooltip

9a13eac

Changed mouse event to custom event

de1babf

Changed mouse event to custom event

c0ea2a8

Updated event type

8b47a3e

Blackbaud-SteveBrush requested a review from Blackbaud-AdamHickey September 11, 2017 14:50

Removed useless comment

79f46e8

Blackbaud-ToddRoberts self-requested a review September 11, 2017 19:04

Blackbaud-ToddRoberts previously requested changes Sep 11, 2017

View reviewed changes

Blackbaud-AdamHickey reviewed Sep 11, 2017

View reviewed changes

styling tweaks

e84fa55

added rounded corners and standardized font & spacing

Blackbaud-ToddRoberts reviewed Sep 11, 2017

View reviewed changes

Blackbaud-ToddRoberts and others added 3 commits September 12, 2017 09:05

remove header divider line

1bc96a8

Merge branch 'component-tooltip' of https://github.com/blackbaud/skyux2…

7ab1b22

… into component-tooltip

Exported directives

c88f991

Blackbaud-AdamHickey approved these changes Sep 13, 2017

View reviewed changes

Blackbaud-SteveBrush added 2 commits September 13, 2017 14:24

Merge branch 'master' into component-tooltip

a2c1ab7

Merged master; added takeUntil

5c567b1

Blackbaud-SteveBrush merged commit 40d1ee7 into master Sep 13, 2017

Blackbaud-SteveBrush deleted the component-tooltip branch September 13, 2017 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Popover Component #1020

Popover Component #1020

Blackbaud-SteveBrush commented Aug 28, 2017

codecov-io commented Aug 28, 2017 •

edited

Loading

Blackbaud-BobbyEarl commented Aug 28, 2017

Blackbaud-ToddRoberts left a comment

Blackbaud-AdamHickey left a comment

Blackbaud-AdamHickey Sep 11, 2017

Blackbaud-AdamHickey Sep 11, 2017

Blackbaud-SteveBrush Sep 12, 2017

Blackbaud-AdamHickey Sep 12, 2017

Blackbaud-AdamHickey Sep 11, 2017

Blackbaud-SteveBrush Sep 12, 2017

Blackbaud-AdamHickey Sep 11, 2017

Blackbaud-ToddRoberts left a comment

Blackbaud-ToddRoberts commented Sep 11, 2017 •

edited

Loading

		@@ -0,0 +1 @@
		export type SkyPopoverPlacement = 'above' \| 'below' \| 'right' \| 'left';

Popover Component #1020

Popover Component #1020

Conversation

Blackbaud-SteveBrush commented Aug 28, 2017

codecov-io commented Aug 28, 2017 • edited Loading

Codecov Report

Blackbaud-BobbyEarl commented Aug 28, 2017

Blackbaud-ToddRoberts left a comment

Choose a reason for hiding this comment

Blackbaud-AdamHickey left a comment

Choose a reason for hiding this comment

Blackbaud-AdamHickey Sep 11, 2017

Choose a reason for hiding this comment

Blackbaud-AdamHickey Sep 11, 2017

Choose a reason for hiding this comment

Blackbaud-SteveBrush Sep 12, 2017

Choose a reason for hiding this comment

Blackbaud-AdamHickey Sep 12, 2017

Choose a reason for hiding this comment

Blackbaud-AdamHickey Sep 11, 2017

Choose a reason for hiding this comment

Blackbaud-SteveBrush Sep 12, 2017

Choose a reason for hiding this comment

Blackbaud-AdamHickey Sep 11, 2017

Choose a reason for hiding this comment

Blackbaud-ToddRoberts left a comment

Choose a reason for hiding this comment

Blackbaud-ToddRoberts commented Sep 11, 2017 • edited Loading

codecov-io commented Aug 28, 2017 •

edited

Loading

Blackbaud-ToddRoberts commented Sep 11, 2017 •

edited

Loading