Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add storybook #46

Draft
wants to merge 28 commits into
base: develop
Choose a base branch
from
Draft

Add storybook #46

wants to merge 28 commits into from

Conversation

MikeTschudi
Copy link
Member

A review-only PR for adding storybook.js to the repo created because components are rendered as text instead of components in the storybook.

It includes a component--arcgis-hub-input-color--copied exactly from hub-components because it contains a known working story. This story is rendered as
image
in this repo instead of
image
as it is rendered in the hub-components storybook.

Only one solutions-components component has a story at this time: solution-item-icon has a trivial story that works when the component is added to the hub-components repo on my computer.

Inside React, components in this repo are handled as HostText instead of HostComponent. As a result, they're added to the storybook as a text block:
image

All development of components and storybook in this repo has been done using Windows 10 computers. Volta was upgraded from 1.0.1 to the latest (1.0.5), and node/npm adjusted to use the latest LTS node and the npm that goes with it. The storybook material was copied from hub-components and modified, and there are library differences between the two repos since I also updated many libraries to their latest versions trying to get this to work.

Thank you, @ajturner & @jmhauck, for offering to look at this!

@ajturner
Copy link
Member

ajturner commented Jan 13, 2022

Looking at this but no luck so far.

I think this is related to why the component isn't rendering in the Addon-Docs either

/cc @tomwayson @mjuniper

@tannerjt
Copy link

@MikeTschudi - I was able to get the preview to work by changing the compiler options in tsconfig-base.json to use es2017 instead of es2020:

{
  "compilerOptions": {
    "lib": ["dom", "es2017"],
    "target": "es2017"
  }
}

Screen Shot 2022-01-13 at 10 46 00 PM

@MikeTschudi
Copy link
Member Author

Thank you @tannerjt! @jmhauck, would you please try this change? It would be great to verify that this problem is only on my computer since that fix doesn't change my display.

@tannerjt
Copy link

@jmhauck -> For verifying, I tried to change back to es2020 to and build and it still worked... so I suspect the step for me that fixed this issue was probably related to changing my volta config in package.json. Here is what I had changed it to for testing (deleting node_modules and re-doing npm install). When I did this locally, I encountered an issue with npm run storybook, but for whatever reason, building the packages with the node/npm version below may have been what actually fixed the issue.

"volta": {
    "node": "14.17.0",
    "npm": "6.14.13"
  }

@jmhauck
Copy link
Collaborator

jmhauck commented Jan 14, 2022

@tannerjt thanks for the suggestions...but I'm getting errors after npm install when building the storybook when I use this.

@MikeTschudi
Copy link
Member Author

I deleted dist & .storybook-static in case caching is a factor, reset to es2017, node 14.17.0, & npm 6.14.13. Build fails with missing references, and so I installed @esri/arcgis-rest-service-admin, @esri/hub-common, & @esri/hub-sites. Finally, had to move arcgis-rest-js, calcite-components, hub.js, and solution.js from peerDependencies to dependencies.

And it works! Thank you, @tannerjt!

(The components continue to work, too! ;-) )

Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

Seems like you should not have had to move them to from peerDependencies to dependencies if they were in devDependencies, so I'm surprised that's what made it work.

package.json Outdated
Comment on lines 72 to 80
"@esri/arcgis-rest-auth": "^3.4.1",
"@esri/arcgis-rest-feature-layer": "^3.4.1",
"@esri/arcgis-rest-geocoding": "^3.4.1",
"@esri/arcgis-rest-portal": "^3.4.1",
"@esri/arcgis-rest-request": "^3.4.1",
"@esri/arcgis-rest-types": "^3.4.1",
"@esri/calcite-components": "1.0.0-beta.74",
"@esri/calcite-ui-icons": "^3.14.1",
"@esri/eslint-plugin-calcite-components": "^0.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

looks like most (if not all) of these are in dependencies, so leaving them here as well will only cause pain and suffering (mismatched versions). You only need them as devDependencies if they are peerDependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't added the new packages to devDependencies; fixed

Comment on lines 15 to 18
const url = new URL(document.currentScript.src);
url.pathname = url.pathname.startsWith('/solutions-components/') ? '/solutions-components/' : '/';
setAssetPath(`${url.href}solutions-components/solutions-components`);
defineCustomElements();
Copy link
Member

Choose a reason for hiding this comment

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

probably best not to use the dist-custom-elements-bundle here, see the changes I made to the Hub Storybook

Copy link
Member Author

Choose a reason for hiding this comment

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

Having trouble with these changes: nothing appears for arcgis-hub-input-color yet; tinkering

Copy link
Member

Choose a reason for hiding this comment

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

The only thought that comes to mind off the top of my head is that that component leans heavily on calcite-components.

@MikeTschudi MikeTschudi marked this pull request as draft February 16, 2022 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants