fix: (core) introduce Card component #3344

dimamarksman · 2020-09-21T08:46:48Z

Please provide a link to the associated issue.

Closes: #3344

Please provide a brief summary of this pull request.

Introduce core standard card 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:

Documentation Examples
Stackblitz works for all examples

netlify · 2020-09-21T09:06:38Z

Deploy preview for fundamental-ngx ready!

Built with commit 59d5451

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

KevinOkamoto

See comments.

KevinOkamoto · 2020-09-21T16:38:38Z

libs/core/src/lib/card/card-footer.component.html

+<fd-toolbar size="compact" [shouldOverflow]="true" fdType="transparent" [clearBorder]="true">
+    <!-- 
+      <ng-content> approach does not work with "overflow" as we have to assign 
+      "fd-toolbar-item" directive to each fd-button to make it working.
+      The ContentChildren(FdButton) list approach with rendering in the template 
+      does not work as we can not access FdButton projected content and render it here
+      withing a loop.
+      We need to find a way to make button projected content available here to keep 
+      control over its rendering in order to make working overflow and force some options
+    -->
+    <ng-content></ng-content>
+</fd-toolbar>


Would it be easier to have the application developer (user) provide the whole fd-toolbar as ng-content instead of trying to internalize here? It's not as convenient, but may be easier for us.

Moved toolbar out of footer, so app developer is responsible to add it properly

KevinOkamoto · 2020-09-21T16:54:24Z

libs/core/src/lib/card/kpi/card-kpi-header.component.ts

+    private _addClassName(element: HTMLElement, className: string): void {
+        this._renderer.addClass(element, className);
+    }


Do we need this extra function? Can we just call this._renderer.addClass(element, className) directly in _addClassNameToHostElement as is done in other components?

Changed it so now it implements cssClassBuilder interface since it was requested by other reviewers

apps/docs/src/app/core/component-docs/card/examples/card-example.component.html

mikerodonnell89 · 2020-09-21T18:43:06Z

apps/docs/src/app/core/component-docs/card/examples/card-compact-example.component.html

+        <fd-card-counter>3 of 10</fd-card-counter>
+    </fd-card-header>
+    <fd-card-content>
+        <ul>


Maybe the card content should display some compact components since there is no discernible difference in this example

Added list in compact mode

mikerodonnell89 · 2020-09-21T20:44:06Z

apps/docs/src/app/core/component-docs/card/examples/card-example.component.html

+        <fd-avatar image="http://lorempixel.com/400/400/nature/4/"></fd-avatar>
+        <fd-card-title>Card Title</fd-card-title>
+        <fd-card-subtitle>Card Subtitle</fd-card-subtitle>
+        <fd-card-counter>1 of 5</fd-card-counter>


Per the styles lib this is actually an "object status". You should be using the fd-object-status directive for this, and add an input to the object status directive called 'isCardCounter' that applies the fd-card__counter class is true.

<span fd-object-status [isCardCounter]="true">1 of 5</span>

I'm not sure it's a good idea to add specific isCardCounter option to fd-object-status.
I believe object-status should know nothing about card component.
Instead we can add status option to fd-card-counter which will support the same values as fd-object-status directive.

What about having @ContentChildren('fd-status-object') in fd-card-header and whenever there is fd-object-status add fd-card__counter class into this element?

I think we should have a predefined selector for that anyway to make it explicit, having fd-object-status that plays a role of card-counter is not obvious and does not look consistently.
So if we have to make it as a directive user needs to add fd-object-status by its own.
If we are going to make it as a component we could hide it in the template of component and expose an input for that.

Taking into account that you suggest stick with a directive approach for the rest staff (title, subtitle) we should make it directive as well, what do you think @JKMarkowski @mikerodonnell89 ?

@dimamarksman I think you can

keep it as compnoent and put fd-object-status into template, or

change it to directive and somehow extend fd-object-status, to keep same inputs. And maybe overwrite cssClassBuilder functions

I changed it to a directive with status input to support object-status modifiers

mikerodonnell89 · 2020-09-21T20:45:53Z

libs/core/src/lib/card/card-counter.component.ts

+import { CLASS_NAME } from './constants';
+
+@Component({
+    selector: 'fd-card-counter',


I think this component can be deleted, see my other comment about fd-object-status

Changed it to a directive, please see my comments above

apps/docs/src/app/core/component-docs/card/examples/card-kpi-example.component.html

mikerodonnell89 · 2020-09-21T21:41:50Z

apps/docs/src/app/core/component-docs/card/examples/card-kpi-example.component.html

+<fd-card cardType="analytical">
+    <fd-card-header
+        >Describes the type of
+        <fd-card-title>Projected revenue</fd-card-title>


I think this should adhere to what we're doing with fd-tile which is to have these be directives rather than components, so instead you'd see <h1 fd-card-title> or <div fd-card-kpi-header>, etc. Maybe the argument could be made that tile should actually be doing what you're doing here, and I'm open to hearing that argument but we should definitely be consistent across these two components as they're very similar

I think it's about following the gitbook specs. It can be for sure transformed to directive, While setting specifications I was almost sure that this fd-card-title will need to keep some additional template, but there is only ng-content. Could you please transform it to directive? Just to let user decide about the markup

Changed it to directives

mikerodonnell89 · 2020-09-21T21:46:45Z

libs/core/src/lib/avatar/avatar.component.ts


    /** Background image resource: url or base64. */
    @Input()
    set image(value: string) {
        this._setImage(value);
    }
+    get image(): string {


I do not understand the purpose of the changes to this file

It is needed to make the avatar image accessble here in the template
https://github.com/SAP/fundamental-ngx/pull/3344/files#diff-df9fe7dae64471bbb9f2e6b9e980a247R7

mikerodonnell89 · 2020-09-21T21:52:02Z

libs/core/src/lib/card/card-loader.component.ts

+
+    /**@hidden */
+    private _addClassNameToHostElement(className: string): void {
+        this._renderer.addClass(this._elementRef.nativeElement, className);


should use cssClassBuilder (same throughout this pr)

If it's only one class you can use event @HostBinding(), but I would like to hear the reason why renderer is used there.

moved to CssClassBuilder

droshev · 2020-09-22T09:55:25Z

This should be feat

dimamarksman · 2020-09-22T10:17:20Z

This should be feat

Sorry, didn't get it

JKMarkowski

Hello @dimamarksman, Great Job! I added couple of comments, let me know if something is not clear.
Also let's consider using CssClassBuilder in directives/components

JKMarkowski · 2020-09-22T09:50:14Z

apps/docs/src/app/core/component-docs/card/examples/card-compact-example.component.html

+            <li>Card content 1</li>
+            <li>Card content 2</li>
+            <li>Card content 3</li>


What about just placing compact list component there?

Added list component

JKMarkowski · 2020-09-22T09:58:30Z

apps/docs/src/app/core/component-docs/card/examples/card-example.component.html

+        <fd-card-counter>1 of 5</fd-card-counter>
+    </fd-card-header>
+    <fd-card-content>
+        <ul>


Could you place list there?

JKMarkowski · 2020-09-22T10:08:13Z

apps/docs/src/app/core/component-docs/card/examples/card-example.component.html

+        <fd-avatar image="http://lorempixel.com/400/400/nature/4/"></fd-avatar>
+        <fd-card-title>Card Title</fd-card-title>
+        <fd-card-subtitle>Card Subtitle</fd-card-subtitle>
+        <fd-card-counter>1 of 5</fd-card-counter>


What about having @ContentChildren('fd-status-object') in fd-card-header and whenever there is fd-object-status add fd-card__counter class into this element?

JKMarkowski · 2020-09-22T10:10:12Z

apps/docs/src/app/core/component-docs/card/examples/card-kpi-example.component.html

+<fd-card cardType="analytical">
+    <fd-card-header
+        >Describes the type of
+        <fd-card-title>Projected revenue</fd-card-title>


I think it's about following the gitbook specs. It can be for sure transformed to directive, While setting specifications I was almost sure that this fd-card-title will need to keep some additional template, but there is only ng-content. Could you please transform it to directive? Just to let user decide about the markup

JKMarkowski · 2020-09-22T10:11:17Z

apps/docs/src/app/core/component-docs/card/examples/card-kpi-example.component.html

+    <fd-card-header
+        >Describes the type of
+        <fd-card-title>Projected revenue</fd-card-title>
+        <fd-card-subtitle>Values include taxes | USD</fd-card-subtitle>


This is same case as title component. It would be great to make it as directive

Changed it to directive

JKMarkowski · 2020-09-22T10:16:56Z

libs/core/src/lib/card/card-second-subtitle.component.ts

+
+import { CLASS_NAME } from './constants';
+
+@Component({


Check my comment regarding title/subtitle

If it's only one class you can use event @HostBinding(), but I would like to hear the reason why renderer is used there.

@HostBinding() creates a binding that should be watched by angular, so if we want to add a static class to the host element only we can do it by renderer, that's more efficient way.

So performace purposes, that's fine for me

Changed it to CssClassBuilder

JKMarkowski · 2020-09-22T10:17:03Z

libs/core/src/lib/card/card-subtitle.component.ts

+import { CLASS_NAME } from './constants';
+
+@Component({
+    selector: 'fd-card-subtitle',


Check my comment regarding title/subtitle

JKMarkowski · 2020-09-22T10:17:23Z

libs/core/src/lib/card/card-title.component.ts

+
+import { CLASS_NAME } from './constants';
+
+@Component({


Check my comment regarding title/subtitle

JKMarkowski · 2020-09-22T10:23:21Z

libs/core/src/lib/card/card.component.ts

+    set cardType(cardType: CardType) {
+        this._setCardType(cardType);
+    }
+    get cardType(): CardType {
+        return this._cardType;
+    }


Wouldn't be much easier for you to use CssClassBuilder?

I did not use CssClassBuilder intentionally cause as far as I know Frank does not like this approach and try to avoid it in the platform project. To be honest I'm not a big fun of this too cause this approach potentially might have some performance issues, sometimes can lead to not obvious bugs

But this is not the platform project so I switched to CssClassBuilder interface.

JKMarkowski · 2020-09-22T10:23:57Z

libs/core/src/lib/card/constants.ts

+export const CLASS_NAME = {
+    card: 'fd-card',
+    cardCompact: 'fd-card--compact',
+    cardHeader: 'fd-card__header',
+    cardTitle: 'fd-card__title',
+    cardSubtitle: 'fd-card__subtitle',
+    cardSecondSubtitle: 'fd-card__second-subtitle',
+
+    cardAnalyticalArea: 'fd-card__analytics-area',
+    cardAnalytics: 'fd-card__analytics',
+    cardAnalyticsText: 'fd-card__analytics-text',
+    cardAnalyticsContent: 'fd-card__analytics-content',
+    cardAnalyticsKpiValue: 'fd-numeric-content__kpi',
+    cardAnalyticsScaleIcon: 'fd-numeric-content__scale-arrow',
+    cardAnalyticsScaleText: 'fd-numeric-content__scale-text',
+
+    cardCounter: 'fd-card__counter',
+    cardBadge: 'fd-card__badge',
+    cardContent: 'fd-card__content',
+    cardFooter: 'fd-card__footer',
+    cardLoader: 'fd-card__loader'
+} as const;
+
+export type CardType = 'standard' | 'component' | 'analytical' | 'list' | 'table' | 'object' | 'quickView' | 'linkList';


I like the idea of it, keeping all of class names in one place

JKMarkowski

LGTM - The only thing you can do there is to change name of spec files. Some of them are still component, when .ts file is directive

dimamarksman · 2020-09-23T14:37:57Z

LGTM - The only thing you can do there is to change name of spec files. Some of them are still component, when .ts file is directive

Thank you!
Spec files have been renamed.

mikerodonnell89

changes look good 👍

KevinOkamoto

Looks good!

InnaAtanasova · 2020-09-24T19:31:12Z

libs/core/src/lib/card/constants.ts

@@ -0,0 +1,25 @@
+export const CLASS_NAME = {


This is very smart!!!!! I like the idea

droshev · 2020-09-25T01:06:07Z

libs/core/src/lib/card/card.component.scss

+@import '~fundamental-styles/dist/numeric-content';
+// @import '~fundamental-styles/dist/card';
+
+// TODO delete it once card styles is released


this can be deleted now since we brought 0.12

droshev

Great Work! We brought styles version 0.12 so you can delete the copy of CSS

* setup card module * [wip] in progress * card [wip] * [wip] * add kpi components * [wip] tests * add tests [wip] * add api files * add onPush * make some components as directives * add object-status to counter * minor * minor * rename unit test files * use fundamental styles * formatting

salarenko requested review from fkolar, KevinOkamoto and a team September 21, 2020 13:58

salarenko added the core Core library specific issues label Sep 21, 2020

salarenko added this to In progress in Development via automation Sep 21, 2020

salarenko added this to the Sprint 46 - Montreal milestone Sep 21, 2020

KevinOkamoto reviewed Sep 21, 2020

View reviewed changes

mikerodonnell89 requested changes Sep 21, 2020

View reviewed changes

Development automation moved this from In progress to Review in progress Sep 21, 2020

JKMarkowski suggested changes Sep 22, 2020

View reviewed changes

dimamarksman force-pushed the introduce-core-standard-card branch from 7e2658c to eb7ce9c Compare September 23, 2020 12:40

JKMarkowski approved these changes Sep 23, 2020

View reviewed changes

mikerodonnell89 self-requested a review September 23, 2020 18:22

mikerodonnell89 approved these changes Sep 23, 2020

View reviewed changes

Development automation moved this from Review in progress to Reviewer approved Sep 23, 2020

KevinOkamoto approved these changes Sep 24, 2020

View reviewed changes

InnaAtanasova changed the title ~~fix: (core) introduce card component~~ fix: (core) introduce Card component Sep 24, 2020

InnaAtanasova reviewed Sep 24, 2020

View reviewed changes

libs/core/src/lib/card/constants.ts

@@ -0,0 +1,25 @@

export const CLASS_NAME = {

Copy link

Contributor

InnaAtanasova Sep 24, 2020

Choose a reason for hiding this comment

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

This is very smart!!!!! I like the idea

InnaAtanasova approved these changes Sep 24, 2020

View reviewed changes

droshev reviewed Sep 25, 2020

View reviewed changes

droshev requested changes Sep 25, 2020

View reviewed changes

dimamarksman added 5 commits September 25, 2020 09:10

setup card module

86fed2b

[wip] in progress

8a71440

card [wip]

4eb8521

[wip]

6f19c24

add kpi components

9069e77

dimamarksman and others added 10 commits September 25, 2020 09:10

[wip] tests

2480873

add tests [wip]

56a7da0

add api files

3427b17

add onPush

d09c983

make some components as directives

45b88b7

add object-status to counter

6b3175a

minor

c1d1a4c

minor

a831b59

rename unit test files

5cb9656

use fundamental styles

0452a1c

dimamarksman force-pushed the introduce-core-standard-card branch from f46e279 to 0452a1c Compare September 25, 2020 07:16

formatting

59d5451

manjunathanagaraj mentioned this pull request Sep 25, 2020

Sprint 46 - Goals #3254

Closed

droshev modified the milestones: Sprint 46 - Montreal, Sprint 46 - ariba Sep 25, 2020

droshev self-requested a review September 25, 2020 11:20

droshev approved these changes Sep 25, 2020

View reviewed changes

droshev merged commit bc0ebca into SAP:master Sep 25, 2020

fix: (core) introduce Card component #3344

fix: (core) introduce Card component #3344

Conversation

dimamarksman commented Sep 21, 2020 • edited by salarenko

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 Sep 21, 2020 • edited

KevinOkamoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimamarksman Sep 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimamarksman Sep 22, 2020 • edited

Choose a reason for hiding this comment

dimamarksman Sep 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimamarksman Sep 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droshev commented Sep 22, 2020

dimamarksman commented Sep 22, 2020

JKMarkowski left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimamarksman Sep 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimamarksman Sep 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimamarksman Sep 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JKMarkowski left a comment • edited

Choose a reason for hiding this comment

dimamarksman commented Sep 23, 2020

mikerodonnell89 left a comment

Choose a reason for hiding this comment

KevinOkamoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droshev left a comment

Choose a reason for hiding this comment

dimamarksman commented Sep 21, 2020 •

edited by salarenko

netlify bot commented Sep 21, 2020 •

edited

dimamarksman Sep 23, 2020 •

edited

dimamarksman Sep 22, 2020 •

edited

dimamarksman Sep 22, 2020 •

edited

dimamarksman Sep 23, 2020 •

edited

JKMarkowski left a comment •

edited

dimamarksman Sep 22, 2020 •

edited

dimamarksman Sep 23, 2020 •

edited

dimamarksman Sep 23, 2020 •

edited

JKMarkowski left a comment •

edited