feat(fw-icon-library): fwIcon extensions and IconLibrary #340

detiwari003 · 2021-12-13T08:45:52Z

…le Imports, Icon Lib & Mutator

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
My commits have standard messages as mentioned in Contributing Guidelines

How Has This Been Tested?

…le Imports, Icon Lib & Mutator

arvindanta · 2021-12-13T09:03:48Z

packages/crayons-core/src/components/icon/icon-utils.ts

+};
+
+// Icon fetch-api with memoization :  fw-icon
+const CRAYONS_ICONS_ASSET_PATH =


At present we are deploying to canary tag. The above url will not work with the current release setup.

arvindanta · 2021-12-13T09:06:50Z

packages/crayons-core/src/components/icon/readme.md

+<code-group>
+<code-block title="HTML">
+```html 
+<fw-icon data-svg={ header } size="20" ></fw-icon>


pls add the code to import header

arvindanta · 2021-12-13T09:10:55Z

packages/crayons-core/src/components/icon/readme.md

+fw-icon can also render external icons. You can use any external libraries from cdn after registering them. If you don't pass 'library' props,
+it will default to 'crayons'. You can even pass the CDN URL of SVG to 'src' prop. See the example below on how to use in React App.
+
+The library registration happpens via a 'ressolver' function. If you wish to apply some mutation to the icons , you may also choose to pass the mutator function.


pls fix the typo

arvindanta · 2021-12-13T09:12:26Z

packages/crayons-core/src/components/icon/readme.md

+
+### Usage in HTML/React Page
+<code-group>
+<code-block title="HTML-FwIconLibrary">


You can keep the title as just HTML and React to stay consistent with other components.
If you want to add variations, pls mention as comments in the code section.

arvindanta · 2021-12-13T09:13:34Z

packages/crayons-core/src/components/icon/readme.md

+
+1. Enable fw-icon as an optimized renderer for SVG with built-in functions such as Intersection-Observer and Fetch-API Memoization.
+   Go through the docs to understand the various props it supports.
+2. React-Support as part of Crayons Release. 


You can remove this statement, since by default we provide React wrapper for all the components.

arvindanta · 2021-12-13T09:15:15Z

packages/crayons-core/src/components/icon/readme.md

+
+Crayons Icon is now available as '@freshworks/crayons-icon' Library. This encapsulates all Icon Tooling icon exports. Following is implemented via the Lib.
+
+1. JS Exports of SVG Icon to enable Tree-Shaking for inline-svg. This is a useful feature where Dev Team can choose to do something offline with SVGs.


Instead of addressing as Dev or development team , you can just address as you.

This is not resolved still. Can you please check

arvindanta · 2021-12-13T09:16:20Z

packages/crayons-core/src/components/icon/readme.md

+
+function App() {
+  return (<div>
+  <FwIcon data-svg={ header } size="20" ></FwIcon>


This would break in a typescript app . Please pass the correct type as value for the props.

arvindanta · 2021-12-13T09:18:26Z

packages/crayons-core/src/components/icon/icon.tsx

+        }
+      } catch (ex) {
+        console.info(
+          `Cannot load ${name}|${library} from CDN. Please check the url & proide a live path.`,


pls fix the typo

arvindanta · 2021-12-13T09:23:08Z

packages/crayons-core/src/components/icon/readme.md

+
+1. JS Exports of SVG Icon to enable Tree-Shaking for inline-svg. This is a useful feature where Dev Team can choose to do something offline with SVGs.
+2. Enable Crayons-Icon lib to support external icon libraries. The development team can register external icon libraries via register API. See usage docs.
+3. Optimized SVGs using SVGO via .yml configuration. The development team can extend the .yml file and use it in their app for optimizing their custom icons. 


Can you provide an example for this ?. How can a consuming app extend and optimize their custom svgs?.

Will need to address after PR Merge.

Added CLI Showcase

arvindanta · 2021-12-13T09:24:26Z

packages/crayons-core/src/components/icon/icon.tsx

-      '--icon-color': this.color,
-    };
+  async loadFallbackImage() {
+    this.svgHTML = await fetchIcon('image', 'crayons', undefined);


You can avoid passing the 3rd argument instead of passing it as undefined.

…m icons ,refactored getSVGState fw-icon

fw-icon

kaushal-freshworks · 2021-12-14T18:40:09Z

packages/crayons-core/src/components/accordion/accordion.stories.mdx

@@ -36,11 +36,11 @@ import { Meta, Story, Preview, Props } from '@storybook/addon-docs/blocks';
                <fw-icon
                  name='minus'
                  size="14"
-                  slot="expanded-icon"></fw-icon>
+                  slot="expanded-icon" library="system" ></fw-icon>


Cant you set it up so that if library is not mentioned , it defaults to using our default icon library ? Basically in your code you set library prop default value to "system" else all components have to be changed as in this PR which has resulted in 57 changed files!

It does not seem possible as we provide crayons icons as the default set. System icon is a subset but we cannot set it to default. Also, there is no way we can make out if it's used from an internal component or an external component. In Shoelace also they are passing library='system' in all internal components.

I tested all those components and its working. I tested by making it fail, then enabling the system mode. We can also do some quick sanity testing by individual developers who worked on that component. They can pull this branch and test it.

…w-icon-library

parsuram-vijaysankar

Minor comments I have added! please check!
but mostly looks good!

parsuram-vijaysankar · 2021-12-15T08:10:55Z

packages/crayons-core/src/components/icon/library.system.ts

+// icons are a subset of Crayons Icons.
+//
+const crayons_system_icons = {
+  'add-contact':


@detiwari003 Shall we restrict this list to have what is used within the crayons component and move out other icons like add-contact!

The ones used inside the readme can point to crayons.

Changed to 10 files. These are those central components which are indispensable.

parsuram-vijaysankar · 2021-12-15T08:12:59Z

packages/crayons-core/src/components/icon/library.system.ts

+  'vertical-align-top':
+    "<svg viewBox='0 0 16 16'><path d='M15.3.1H.7a.7.7 0 1 0 0 1.4H2v13.7a.7.7 0 0 0 .7.7h3.6a.7.7 0 0 0 .7-.7V1.5h2v5.7a.7.7 0 0 0 .7.7h3.6a.7.7 0 0 0 .7-.7V1.5h1.3a.7.7 0 0 0 0-1.4zM5.6 14.5H3.4v-13h2.2v13zm7-8h-2.2v-5h2.2v5z' fill='currentColor' fill-rule='evenodd'/></svg>",
+  'warning':
+    "<svg viewBox='0 0 16 16'><path d='M8 0a8 8 0 1 1-8 8 8 8 0 0 1 8-8zm0 1.4A6.6 6.6 0 1 0 14.6 8 6.6 6.6 0 0 0 8 1.4zm.05 8.66a1 1 0 0 1 1.05 1.05 1.05 1.05 0 1 1-1.05-1.05zm.35-5.966a.7.7 0 0 1 .35.606v3.61a.7.7 0 0 1-1.4 0V4.7a.7.7 0 0 1 1.05-.606z' fill='#e43538'/></svg>",


Let's not theme at the base level. I understand that this was originally done in the icon, but lets change it.
fill='#e43538' => fill='currentColor'

parsuram-vijaysankar · 2021-12-15T08:19:16Z

packages/crayons-core/crayons-react/index.ts

Lets rename
fwIconUnregisterLibrary -> registerLibrary
fwIconUnregisterLibrary -> unregisterLibrary

arvindanta · 2021-12-15T08:30:01Z

packages/crayons-core/src/components/accordion/readme.md

      name='plus'
      size="14"
-      slot="collapsed-icon"></fw-icon>
+      slot="collapsed-icon" system library="system" ></fw-icon>


can u remove the attribute system

arvindanta · 2021-12-15T08:31:43Z

packages/crayons-core/src/components/icon/icon.tsx

+    if (hasLabel) {
+      accessibilityProps['role'] = 'img';
+      accessibilityProps['aria-label'] = this.label;
+      accessibilityProps['aria-hidden'] = undefined;


do we need to explicity set the value as undefined?

set to true. Fixed

arvindanta · 2021-12-15T08:34:34Z

packages/crayons-core/src/components/icon/readme.md

+1. Enable fw-icon as an optimized renderer for SVG with built-in functions such as Intersection-Observer and Fetch-API Memoization.
+   Go through the docs to understand the various props it supports.
+2. Providing icon-support for crayons-system components and also exposing crayons-icon set for public use with inbuilt support for external icon-libraries also.
+3. Icons are accessibility tree compliant. You need to pass the label attribute. aria-hidden is set to true.


You have set aria-hidden to undefined above. Here it is mentioned it as being set to true. Can you pls check .

arvindanta · 2021-12-15T08:35:28Z

packages/crayons-core/src/components/icon/readme.md

@@ -23,28 +30,188 @@ import ReactDOM from "react-dom";
 import { FwIcon } from "@freshworks/crayons/react";
 function App() {
  return (<div>
-  <FwIcon name="add-contact" size="18" color="green"></FwIcon>
+  <FwIcon name="add-contact" size="18" color="green" ></FwIcon>


This will result in typeerror in a Typescript app. Can you please correct it ?

arvindanta · 2021-12-15T08:35:44Z

packages/crayons-core/src/components/icon/readme.md

-### Icons
+### Intersection Observer
+
+Use prop 'lazy' to enable Intersection-Observer. By deafult it is disabled. You may choose to give the intersection root-margin for icons i.e via prop 'x-root-margin' as preloading threshold in a viewport.Default value is '50px'.


Please fix the typo

arvindanta · 2021-12-15T17:16:44Z

packages/crayons-core/src/components/icon/icon.tsx

+          throw 'Icon-URL Not Valid.';
+        }
+      } catch (ex) {
+        console.info(


You are throwing an error with a message but in the below catch block , you are not logging that error but instead just having a static error description.

detiwari003 · 2021-12-16T17:17:17Z

Minor comments I have added! please check! but mostly looks good!

Thanks Parsu. I have now closed all review comments and tested them.

parsuram-vijaysankar

LGTM!

There is one minor comment that you need to address.

parsuram-vijaysankar · 2021-12-16T17:29:10Z

www/css-utils/card/readme.md

@@ -21,7 +21,7 @@ To apply card css utils, we can use 'fw-card-#{$elevation}'
        </div>
        <div class="fw-flex-grow-0">
          <fw-button size="icon" color="text" role="button" class="fw-ml-12">
-            <fw-icon name="delete"></fw-icon>
+            <fw-icon name="delete" library="system" ></fw-icon>


Delete and inbox will be part of the crayons library, right? let's change this otherwise, the documentation will be wrong.

arvindanta · 2021-12-16T17:41:29Z

packages/crayons-core/src/components/icon/icon.tsx

+    if (hasLabel) {
+      accessibilityProps['role'] = 'img';
+      accessibilityProps['aria-label'] = this.label;
+      accessibilityProps['aria-hidden'] = true;


Should we not set the aria-hidden to true by default for icons. Here if the label is not passed, aria-hidden attribute will not be set.

arvindanta · 2021-12-16T17:51:53Z

package.json

@@ -74,6 +74,7 @@
    "stylelint-prettier": "^1.2.0",
    "stylelint-scss": "^3.21.0",
    "vuepress": "^1.8.2",
+    "vuepress-plugin-demo-code": "^0.6.0",


@detiwari003 Is this dependency required ?. Where is it being used in the docs site ?. Can you please check.

arvindanta

LGTM except for the couple of items regarding a11y and a new dependency being added. kindly check on them.

…w-icon-library

feat(fw-icon): fwIcon changes i.e Intersection-Observer, Tree shakeab…

71805d5

…le Imports, Icon Lib & Mutator

detiwari003 requested review from parsuram-vijaysankar and arvindanta December 13, 2021 08:45

arvindanta reviewed Dec 13, 2021

View reviewed changes

detiwari003 added 3 commits December 14, 2021 10:02

feat(fw-icon-library): added system.default, crayons components>syste…

5d88560

…m icons ,refactored getSVGState fw-icon

fix(test): test

d23f88e

fix(readme fixes. examples correction): fw-icon

b56e604

detiwari003 requested review from arvindanta and kaushal-freshworks December 14, 2021 06:46

fix(fwicon): file reorg and readMe changes

d9d274a

fw-icon

kaushal-freshworks reviewed Dec 14, 2021

View reviewed changes

detiwari003 added 2 commits December 15, 2021 12:08

Merge branch 'canary' of https://github.com/freshworks/crayons into f…

98e3b92

…w-icon-library

fix(icon): delete a wrong placed in file

d75ce33

parsuram-vijaysankar suggested changes Dec 15, 2021

View reviewed changes

arvindanta reviewed Dec 15, 2021

View reviewed changes

detiwari003 requested a review from parsuram-vijaysankar December 16, 2021 05:02

detiwari003 requested review from kaushal-freshworks and arvindanta December 16, 2021 05:02

fix(review comments closure): review Comments Closure

a3560fd

detiwari003 self-assigned this Dec 16, 2021

detiwari003 added 2 commits December 16, 2021 22:39

feat(crayons-icon-cli): crayons Icon CLI Tooling

314d84a

fix(try-catch): try-catch

adab591

detiwari003 closed this Dec 16, 2021

detiwari003 reopened this Dec 16, 2021

parsuram-vijaysankar approved these changes Dec 16, 2021

View reviewed changes

fix(readme): readme

d5fa87d

arvindanta reviewed Dec 16, 2021

View reviewed changes

arvindanta requested changes Dec 16, 2021

View reviewed changes

arvindanta approved these changes Dec 16, 2021

View reviewed changes

detiwari003 added 3 commits December 17, 2021 07:59

Merge branch 'canary' of https://github.com/freshworks/crayons into f…

7627ada

…w-icon-library

fix(corrections): readMe

b1d5ee5

fix(readme): read me

3015a7f

detiwari003 merged commit 9e5ceb3 into canary Dec 17, 2021

detiwari003 deleted the fw-icon-library branch December 17, 2021 06:04


		Crayons Icon is now available as '@freshworks/crayons-icon' Library. This encapsulates all Icon Tooling icon exports. Following is implemented via the Lib.

		1. JS Exports of SVG Icon to enable Tree-Shaking for inline-svg. This is a useful feature where Dev Team can choose to do something offline with SVGs.

feat(fw-icon-library): fwIcon extensions and IconLibrary #340

feat(fw-icon-library): fwIcon extensions and IconLibrary #340

Conversation

detiwari003 commented Dec 13, 2021 • edited Loading

Checklist:

How Has This Been Tested?

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

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

arvindanta Dec 13, 2021 • edited Loading

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

parsuram-vijaysankar left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detiwari003 commented Dec 16, 2021

parsuram-vijaysankar 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

Choose a reason for hiding this comment

arvindanta Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindanta left a comment

Choose a reason for hiding this comment

detiwari003 commented Dec 13, 2021 •

edited

Loading

arvindanta Dec 13, 2021 •

edited

Loading

parsuram-vijaysankar left a comment •

edited

Loading

arvindanta Dec 16, 2021 •

edited

Loading