feat: (Core) introduce Action sheet component #3596

katekozlowska · 2020-10-12T07:39:49Z

Please provide a link to the associated issue.

closes 3372

Please provide a brief summary of this pull request.

Introduce core action sheet component

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/master/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist

Documentation checklist:

Breaking Changes Wiki
Documentation Examples
Stackblitz works for all examples

netlify · 2020-10-12T07:42:28Z

Deploy preview for fundamental-ngx ready!

Built with commit e407ca8

https://deploy-preview-3596--fundamental-ngx.netlify.app

apps/docs/src/app/core/component-docs/action-sheet/action-sheet-docs.component.html

libs/core/src/lib/action-sheet/action-sheet-item/action-sheet-item.component.html

libs/core/src/lib/action-sheet/action-sheet.component.ts

mikerodonnell89 · 2020-10-12T19:35:03Z

Because this is essentially a list, I believe the items should be keyboard navigable

mikerodonnell89 · 2020-10-12T19:36:10Z

We need a way to close the mobile view

mikerodonnell89 · 2020-10-12T19:39:43Z

The fiori designs have the popover w/ the arrow. Also the popover should be centered under the button rather than aligned to the side

mikerodonnell89

Code looks good, just needs some small changes, see comments

JKMarkowski

I totally agree with mike, that you should also add some keyboard support. Key Up/Down + anytime popover is opened the first option shoudl be focused

apps/docs/src/app/core/component-docs/action-sheet/action-sheet-docs.component.html

...-docs/action-sheet/examples/action-sheet-compact/action-sheet-compact-example.component.html

...nt-docs/action-sheet/examples/action-sheet-compact/action-sheet-compact-example.component.ts

...omponent-docs/action-sheet/examples/action-sheet-cosy/action-sheet-cosy-example.component.ts

...nent-docs/action-sheet/examples/action-sheet-mobile/action-sheet-mobile-example.component.ts

libs/core/src/lib/action-sheet/action-sheet.component.ts

libs/core/src/lib/action-sheet/action-sheet.component.html

libs/core/src/lib/action-sheet/action-sheet.component.ts

libs/core/src/lib/action-sheet/action-sheet.interface.ts

InnaAtanasova

Awesome work! The rest of the reviewers should verify if their requested changes have been addressed but from my part it looks great.

libs/core/src/lib/action-sheet/action-sheet.component.ts

apps/docs/src/app/core/core-documentation.module.ts

apps/docs/src/app/documentation/utilities/consts/mobile-mode-configuration-tokens.ts

libs/core/src/lib/action-sheet/action-sheet-item/action-sheet-item.component.ts

JKMarkowski

Hi @katekozlowska I added some comments, also one of the buttons in compact mode doesn't close popover, even if it has [isCloseButton]="true"

libs/core/src/lib/action-sheet/action-sheet-mobile/action-sheet-mobile.component.ts

libs/core/src/lib/action-sheet/action-sheet.component.ts

libs/core/src/lib/utils/base-class/mobile-mode.class.ts

libs/core/src/lib/popover/popover.module.ts

JKMarkowski

I found some leftovers / unguarded subscription. After addressing it, we should be good to go with this PR

JKMarkowski · 2020-10-27T13:02:32Z

libs/core/src/lib/action-sheet/action-sheet.component.ts

+        this._onDestroy$.next();
+        this._onDestroy$.complete();
+        this.actionSheetControl.clicked.unsubscribe();
+        // this.actionSheetItems.unsubscribe();


JKMarkowski · 2020-10-27T13:04:17Z

libs/core/src/lib/action-sheet/action-sheet.component.ts

+
+    /** @hidden */
+    private _actionItemsHandle(): void {
+        this.actionSheetItems.forEach(item => item.clicked.subscribe((isOpen) => this.isOpenChangeHandle(isOpen))


Can you unsubscribe after destroy there? You can do it by adding .pipe(takeUntil(this._onDestroy$)) like in other places in code

JKMarkowski · 2020-10-27T13:04:22Z

libs/core/src/lib/action-sheet/action-sheet.component.ts

+
+    /** @hidden */
+    private _actionControlHandle(): void {
+        this.actionSheetControl.clicked.subscribe((isOpen) => this.isOpenChangeHandle(isOpen));


Can you unsubscribe after destroy there? You can do it by adding .pipe(takeUntil(this._onDestroy$)) like in other places in code

JKMarkowski · 2020-10-27T13:05:02Z

libs/core/src/lib/action-sheet/action-sheet.component.ts

+    ngOnDestroy(): void {
+        this._onDestroy$.next();
+        this._onDestroy$.complete();
+        this.actionSheetControl.clicked.unsubscribe();


I'm not sure if it's going to work. After checking my other comment about unsubscribing you would be able to remove this line

JKMarkowski

LGTM 🚀

libs/core/src/lib/popover/popover.module.ts

apps/docs/src/app/core/component-docs/action-sheet/index.ts

katekozlowska and others added 8 commits October 1, 2020 13:29

initial commit

05c0935

action sheet structure and update

74c9590

save

2ff468c

mobile example

f4dbdd2

adding styles to compact mode

c4a1309

fd-action-item as component

9be6dcf

clearout

0a415e9

mobile and compact mode fixes

ddf579c

katekozlowska changed the title ~~Feat/action sheet~~ Action Sheet Oct 12, 2020

katekozlowska changed the title ~~Action Sheet~~ Feat - Action Sheet Oct 12, 2020

Betrozov suggested changes Oct 12, 2020

View reviewed changes

katekozlowska changed the title ~~Feat - Action Sheet~~ feat - (Core) introduce Action sheet component Oct 12, 2020

katekozlowska self-assigned this Oct 12, 2020

katekozlowska added this to the Sprint 48 - Bora Bora milestone Oct 12, 2020

katekozlowska added the New Fiori component label Oct 12, 2020

katekozlowska added 2 commits October 12, 2020 14:03

fixes

e585054

fixes

02c94f0

mikerodonnell89 requested changes Oct 12, 2020

View reviewed changes

JKMarkowski suggested changes Oct 13, 2020

View reviewed changes

katekozlowska added 7 commits October 14, 2020 07:42

addressing PR comments

d382f93

PR comments

7e13430

PR comments

2f920e3

PR comments

17df384

navigate on keydown

21559e8

keydown naviagtion and clearout

cefb05e

fix path

7d8624e

InnaAtanasova approved these changes Oct 24, 2020

View reviewed changes

InnaAtanasova modified the milestones: Sprint 48 - Bora Bora, Sprint 49 - Nassau Oct 24, 2020

InnaAtanasova added the denoland label Oct 24, 2020

katekozlowska added 2 commits October 26, 2020 10:02

PR comments, clearout

2dee72f

build fix

2fb05c9

salarenko suggested changes Oct 26, 2020

View reviewed changes

PR cmts

076653a

InnaAtanasova requested a review from salarenko October 26, 2020 12:09

InnaAtanasova added this to In progress in Development via automation Oct 26, 2020

fix mobile click, PR comments

7cf3d51

JKMarkowski suggested changes Oct 27, 2020

View reviewed changes

katekozlowska added 3 commits October 27, 2020 12:14

clearout

5c21047

remove leftovers

2085365

fixes

0098786

katekozlowska requested a review from JKMarkowski October 27, 2020 12:38

JKMarkowski suggested changes Oct 27, 2020

View reviewed changes

add unsubscribe

b9b4d46

JKMarkowski approved these changes Oct 27, 2020

View reviewed changes

salarenko suggested changes Oct 28, 2020

View reviewed changes

libs/core/src/lib/popover/popover.module.ts Outdated Show resolved Hide resolved

apps/docs/src/app/core/component-docs/action-sheet/index.ts Outdated Show resolved Hide resolved

mikerodonnell89 approved these changes Oct 29, 2020

View reviewed changes

fix(core) fix indexing | Revert popover module changes

15e2b1f

salarenko approved these changes Oct 30, 2020

View reviewed changes

mikerodonnell89 and others added 4 commits October 30, 2020 11:26

fix conflict

32ae8d6

build fix

b392ebd

Merge branch 'master' into feat/action-sheet

fa2c461

clearout

e407ca8

katekozlowska merged commit 5194d67 into master Nov 2, 2020

Development automation moved this from In progress to Done Nov 2, 2020

katekozlowska deleted the feat/action-sheet branch November 2, 2020 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: (Core) introduce Action sheet component #3596

feat: (Core) introduce Action sheet component #3596

katekozlowska commented Oct 12, 2020 •

edited

netlify bot commented Oct 12, 2020 •

edited

mikerodonnell89 commented Oct 12, 2020

mikerodonnell89 commented Oct 12, 2020

mikerodonnell89 commented Oct 12, 2020 •

edited

mikerodonnell89 left a comment

JKMarkowski left a comment

InnaAtanasova left a comment

JKMarkowski left a comment •

edited

JKMarkowski left a comment

JKMarkowski Oct 27, 2020

JKMarkowski Oct 27, 2020

JKMarkowski Oct 27, 2020

JKMarkowski Oct 27, 2020

JKMarkowski left a comment

feat: (Core) introduce Action sheet component #3596

feat: (Core) introduce Action sheet component #3596

Conversation

katekozlowska commented Oct 12, 2020 • edited

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Please check whether the PR fulfills the following requirements

netlify bot commented Oct 12, 2020 • edited

mikerodonnell89 commented Oct 12, 2020

mikerodonnell89 commented Oct 12, 2020

mikerodonnell89 commented Oct 12, 2020 • edited

mikerodonnell89 left a comment

Choose a reason for hiding this comment

JKMarkowski left a comment

Choose a reason for hiding this comment

InnaAtanasova left a comment

Choose a reason for hiding this comment

JKMarkowski left a comment • edited

Choose a reason for hiding this comment

JKMarkowski left a comment

Choose a reason for hiding this comment

JKMarkowski Oct 27, 2020

Choose a reason for hiding this comment

JKMarkowski Oct 27, 2020

Choose a reason for hiding this comment

JKMarkowski Oct 27, 2020

Choose a reason for hiding this comment

JKMarkowski Oct 27, 2020

Choose a reason for hiding this comment

JKMarkowski left a comment

Choose a reason for hiding this comment

katekozlowska commented Oct 12, 2020 •

edited

netlify bot commented Oct 12, 2020 •

edited

mikerodonnell89 commented Oct 12, 2020 •

edited

JKMarkowski left a comment •

edited