Implemented `EdsIcon` component #227

rubenthoms · 2022-08-01T14:27:05Z

This PR introduces a new React component named EdsIcon. The component accepts up to three properties: icon, color and size. With this component, icons from the Equinor Design System (EDS) can be used directly in Python.

This PR closes #224.

valentin-krasontovitsch

just a few questions from somebody who is new to typescript...

valentin-krasontovitsch · 2022-08-01T16:08:57Z

react/src/lib/components/EdsIcon/EdsIcon.jsx

+    icon: PropTypes.string.isRequired,
+    size: PropTypes.oneOf([16, 24, 32, 40, 48]),
+    color: PropTypes.string,
+};


so how come you have to define the propTypes twice? and why does one need this very thin jsx wrapper around the tsx file? I'm sorry, I'm rather new to typescript...

This is in order to make dash-generate-components recognize the component when creating the Python wrapper around it. With the newer versions of Dash it should be able to recognize TypeScript, but there are still some issues with the newest Dash version so we cannot use it yet.

As you can see, we followed this pattern for all components in wcc. By the time the new solution is in place, we are going to adjust the whole package (including operations in the package.json). If you wonder why we're defining TypeScript and PropTypes, they are not doing exactly the same. TypeScript is checked at build time while PropTypes are checked at runtime. Since we don't know the exact context in which the components are going to be used (might be in e.g. an API with types coming in at runtime), we are equipping our components with both, build and runtime type checking.

ah, cool! thanks for the explanation!

valentin-krasontovitsch · 2022-08-01T16:09:20Z

react/src/lib/components/EdsIcon/EdsIcon.jsx

@@ -0,0 +1,13 @@
+import React from "react";


is this import statement necessary? I don't see where you use React below.

This seems to be a leftover from the copy-paste operation and can be removed ;)

ah, yummy copypasta, my favorite ^^

I stay corrected: before, each JSX component had to be in the React scope - hence, React must be imported, no matter if it is used or not. This is because the JSX file is converted to a JavaScript file in which the React.createElement is used (see here: https://reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html). The new JSX transform does not require this anymore, but we don't have the setup adjusted yet. Hence, we have to keep it for now.

aha, right! that makes sense, I read about the JSX and how it's transformed. cool! thanks for the further explanation ^^

valentin-krasontovitsch

Apart from the (for me dead?) link and my question, this looks good.

Sadly I wasn't able to inject the to be merged version of webviz-core-components into our app (using a simple pip install from github command, the setup script couldn't find package.json) to test the component live, in action, so I'm gonna rely on you having tried out what it actually looks like : )

Thank you very much!

valentin-krasontovitsch · 2022-08-08T10:13:39Z

react/src/lib/components/EdsIcon/components/EdsIcon.tsx

+                    `An icon with name "${props.icon}" does not exist.` +
+                    ` Please check the icon name for typos. ` +
+                    `An overview of all available icons can be found at ` +
+                    `https://eds.equinor.com/assets/system-icons/library/.`


that link is a 404, at least for me from an unmanaged laptop connected to Statoil-Approved

i managed to access the storybook for icons though, under

https://eds-storybook-react.azurewebsites.net/?path=/story/icons--preview

Yep, seems like EDS changed their website. I will adjust the link. Thanks for the catch!

no worries : )

valentin-krasontovitsch · 2022-08-08T10:18:26Z

react/src/lib/components/EdsIcon/components/EdsIcon.tsx

+    color?: string;
+};
+
+export const EdsIcon: React.FC<IconProps> = (props) => {


I'm a complete noob when it comes to typescript, and haven't worked much with React either, so this review is probably at most a chance for me to learn something, but anyways - I would have expected the props lambda function argument to be typed, but I suspect that the type is indirectly encoded in React.FC<IconProps>? : )

Yes, that is correct.

cool, thanks!

rubenthoms · 2022-08-08T11:53:04Z

@valentin-krasontovitsch you can test a webviz-core-components branch by checking it out, then starting a terminal in the respective directory and running

npm run build --prefix react/
pip install -e .

valentin-krasontovitsch · 2022-08-08T12:14:34Z

ah cool thank you! gonna try it out : )

valentin-krasontovitsch · 2022-08-08T12:16:15Z

and of course:

sh: 1: tsc: not found

: D

valentin-krasontovitsch · 2022-08-08T12:25:11Z

okay i think i'm doing something wrong here.

cloned from github
checked out the respective branch
ran npm run build --prefix react/ in root folder of github project (after installing typescript)
tried pip install -e ., still in project root, getting following error:

Obtaining file:///home/val/code/equinor/webviz-core-components
  Preparing metadata (setup.py) ... error
  error: subprocess-exited-with-error

  × python setup.py egg_info did not run successfully.
  │ exit code: 1
  ╰─> [6 lines of output]
      Traceback (most recent call last):
        File "<string>", line 2, in <module>
        File "<pip-setuptools-caller>", line 34, in <module>
        File "/home/val/code/equinor/webviz-core-components/setup.py", line 8, in <module>
          with open("webviz_core_components/package.json", encoding="utf8") as f:
      FileNotFoundError: [Errno 2] No such file or directory: 'webviz_core_components/package.json'

any pointers?

rubenthoms · 2022-08-08T12:36:25Z

@valentin-krasontovitsch ah sorry, I forgot that you did not have any setup at all. Before running for the first time, you have to install both all required npm packages and Python packages. In addition, you need a version of Node.js (do you have one installed? - if not I'd recommend to use NVM (https://github.com/nvm-sh/nvm) - a Node Version Manager - and install the latest LTS version of Node.js - 16.16.0 at the moment: https://nodejs.org/en/). When Node.js is installed, you can run

npm ci --ignore-scripts --prefix ./react
pip install .[dependencies] 
pip install dash[dev]

Then the initial setup is done and you can use the commands I mentioned further up.

valentin-krasontovitsch · 2022-08-08T13:04:45Z

tried it, but still the second command complains with the same error as above 🤔

valentin-krasontovitsch · 2022-08-08T13:44:40Z

just for future prosperity - in a video meet, we found out that we should copy react/package.json into webviz_core_components/ (all paths relative to project root)

tested functionality manually and visually, looking great, nonsensical icon text yields desired warning icon with help text. coolio!

valentin-krasontovitsch · 2022-08-08T13:45:55Z

oh also needed to use dash version 2.4, 2.5 and 2.6 have known issues

jorgenherje

Tested and verified. Look good! 👍

Implemented EdsIcon component.

44320fd

rubenthoms added the enhancement New feature or request label Aug 1, 2022

rubenthoms requested a review from anders-kiaer August 1, 2022 14:27

rubenthoms self-assigned this Aug 1, 2022

Adjusted changelog.

15d7db8

valentin-krasontovitsch reviewed Aug 1, 2022

View reviewed changes

rubenthoms force-pushed the EQ_224-make-icons-available branch from 986302d to 15d7db8 Compare August 1, 2022 17:55

rubenthoms requested a review from valentin-krasontovitsch August 4, 2022 14:25

valentin-krasontovitsch approved these changes Aug 8, 2022

View reviewed changes

Adjusted deprecated link to EDS icon webpage.

0bec201

Merge branch 'master' into EQ_224-make-icons-available

36fd46e

rubenthoms requested a review from jorgenherje August 9, 2022 08:40

rubenthoms and others added 2 commits August 9, 2022 10:46

Merge branch 'master' into EQ_224-make-icons-available

8356c34

Removed JSX file, add id property, added story

1377f51

valentin-krasontovitsch mentioned this pull request Aug 9, 2022

Plot Viewer: Transform Buttons into icons, and reposition equinor/webviz-ert#333

Closed

jorgenherje approved these changes Aug 9, 2022

View reviewed changes

rubenthoms merged commit 198092d into equinor:master Aug 9, 2022

Implemented EdsIcon component #227

Implemented EdsIcon component #227

Uh oh!

Conversation

rubenthoms commented Aug 1, 2022

Uh oh!

valentin-krasontovitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentin-krasontovitsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubenthoms commented Aug 8, 2022

Uh oh!

valentin-krasontovitsch commented Aug 8, 2022

Uh oh!

valentin-krasontovitsch commented Aug 8, 2022

Uh oh!

valentin-krasontovitsch commented Aug 8, 2022

Uh oh!

rubenthoms commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valentin-krasontovitsch commented Aug 8, 2022

Uh oh!

valentin-krasontovitsch commented Aug 8, 2022

Uh oh!

valentin-krasontovitsch commented Aug 8, 2022

Uh oh!

jorgenherje left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Implemented `EdsIcon` component #227

Implemented `EdsIcon` component #227

rubenthoms commented Aug 8, 2022 •

edited

Loading