Added Storybook using JSX with UV demos #720
Conversation
There was a problem hiding this comment.
This looks great based on the screen shot, but I wasn't able to get it to work on my end. I checked out the code, ran npm install ; npm run-script build ; npm run storybook and it pops up in my browser, but the page never fully loads. I get the storybook framework, but the left sidebar just stays forever in a blinking loading state.
I'm not familiar with this tool, so maybe I'm just missing something, but it's not clear to me how the changes in this pull request would expose everything shown in the screen shot. For example, how do you specify which knobs to expose through the interface? This leads me to wonder if my problem has to do with a missing file that hasn't been committed, or something like that.
I also had a couple of small questions and comments which are separated out below. Of these, I think the most significant is the include vs. require thing; I'm just not sure why that change is necessary, and if nothing else, the associated comment should be made more detailed. :-)
| @@ -14,7 +14,8 @@ import { Helper, loadManifest, IManifoldOptions } from "@iiif/manifold"; | |||
| import { Annotation, AnnotationBody, Canvas, Sequence } from "manifesto.js"; | |||
| import { BaseComponent, IBaseComponentOptions } from "@iiif/base-component"; | |||
| import { URLDataProvider } from "./URLDataProvider"; | |||
| import "./lib/"; | |||
| // Has to be require. | |||
| require("./lib/"); | |||
There was a problem hiding this comment.
I think this was related to a bug when accessing globals like $?
There was a problem hiding this comment.
Whatever the reason, I think it would be a good idea to understand what is going on. My understanding of include vs. require suggests that the differences between them are minimal (the biggest difference is that require can accept dynamic inputs, while include cannot -- which makes include better for tree-shaking because it is predictable). If using include instead of require on a static input is causing a change in behavior, that's probably a side effect of some step in the compilation process related to Typescript and/or Webpack, and perhaps there's a different/better solution, or there are other implications.
Anyway, I imagine there's a very good chance that @stephenwf has already thought this all through, but if that's the case, I'd just like to see more of that thinking captured in the comment so we don't have to re-investigate in the future when we come back around and wonder why exactly we need a require here.
There was a problem hiding this comment.
In short, the problem was the loading order. There's nothing in ./lib/ that includes jQuery or jQuery views as a dependency. A more involved fix for this would be to move more into the webpack build, like jQuery and the plugins so that Webpack can ensure the correct loading order etc.
In terms of output, the only difference in the bundle is the removal of a hint left by Webpack for minification:
- /* harmony import */ var _lib___WEBPACK_IMPORTED_MODULE_7__ = __webpack_require__(/*! ./lib/ */ "./src/lib/index.js");
+ __webpack_require__(/*! ./lib/ */ "./src/lib/index.js");There was a problem hiding this comment.
Interesting. Maybe the comment could be changed to something like Must be require instead of include to support jQuery; TODO: eliminate jQuery or explicitly include it?
| @@ -0,0 +1,2 @@ | |||
| <script src="https://unpkg.com/[email protected]/dist/ResizeObserver.js"></script> | |||
There was a problem hiding this comment.
It would sure be nice to figure out a way to not have to put this everywhere that bundle.js is loaded! (But that's a problem for another day...)
| rotation?: number, | ||
| xywh?: string, | ||
| embedded?: boolean, | ||
| locales?: any, //?? |
There was a problem hiding this comment.
Does this ?? comment indicate that further work/discussion is needed here?
There was a problem hiding this comment.
I think so, as far as I can tell, there are no types for the locale config in the UV.
There was a problem hiding this comment.
@edsilv, any thoughts on this? Hopefully we can either adjust the type (if necessary/possible) or remove the comment (if no further action is needed).
There was a problem hiding this comment.
I think typing it shouldn't be a problem
|
@demiankatz I was having some problems with the files being gitignored! Looks like the storybook config isn't there. Let me change that now 👍 |
| module.exports = { | ||
| stories: ['../stories/**/*.stories.tsx'], | ||
| addons: ['@storybook/addon-knobs/register'], | ||
| webpackFinal: async config => { | ||
| config.module.rules.push({ | ||
| test: /\.(ts|tsx)$/, | ||
| use: [ | ||
| { | ||
| loader: require.resolve('ts-loader'), | ||
| }, | ||
| // Optional | ||
| { | ||
| loader: require.resolve('react-docgen-typescript-loader'), | ||
| }, | ||
| ], | ||
| }); | ||
| config.resolve.extensions.push('.ts', '.tsx'); | ||
| return config; | ||
| }, | ||
| }; |
There was a problem hiding this comment.
@demiankatz Here's the missing file, you should be able to run it locally now. Sorry about that, problems with the .gitignore for main.js files.
There was a problem hiding this comment.
Well, that's progress, but it's still not quite working for me. Now, if I run grunt build and npm run storybook, I see the full storybook interface with the two examples you provided and the appropriate knobs, but the UV itself doesn't load. The pane where it should be is white and empty. I'm seeing a 404 error in my console for the file http://localhost:6006/uv/lib/uv-openseadragon-extension.en-GB.config.json.
I haven't tried deleting everything and starting fresh with a clean npm install. If you think that's worth a shot, let me know and I'll do it... but since it's a time-consuming process, I first wanted to check and see if the missing JSON file was an obvious indication of a different problem.
There was a problem hiding this comment.
For what it's worth, on the working Netlify demo you shared, the config.json file is coming from the URL https://uv-storybook.netlify.com/uv-assets/config/uv-openseadragon-extension.en-GB.config.json. Not sure why the path is so significantly different in my local environment.
There was a problem hiding this comment.
Hmm, can you try: grunt build --dist and then npm run storybook ?
There was a problem hiding this comment.
Same result. I have a ./dist directory which is being served up by Storybook through port 6006... but something is still trying to request http://localhost:6006/uv/lib/uv-openseadragon-extension.en-GB.config.json and there's no ./dist/uv directory in my instance, so I get a 404.
There was a problem hiding this comment.
it's weird. runs fine with npm start
There was a problem hiding this comment.
@edsilv, did you mean grunt build --dist? I tried running npm build --dist and just got an error message.
There was a problem hiding this comment.
@edsilv, I don't get an error when I do that, but I don't think the --dist flag carries through when you wrap npm around the command.
There was a problem hiding this comment.
(I think npm run build:dist is the way to do it through npm as currently configured. But that works for me too. Perhaps the virtex error you're seeing has to do with an artifact in your local system? Did you try from a clean checkout?
Support for Storybook using the HTML preset and JSX-DOM for rendering the HTML using JSX.
npm buildfirstnpm run storybook./distso you can access configs and resources in that folder – before we switch that over the webpack too.HTMLElementthat can be immediately attached. (not React)./stories/and can pull in files, classes from../srcrelative to that folder.libfolder. You'll have to restart to see these changes.Mostly a baseline, I wanted to get the minimum configuration in, plenty more could be added for theming, add-ons and showcasing various other UI elements in isolation.