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

Added Storybook using JSX with UV demos #720

Open
wants to merge 2 commits into
base: webpack
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .storybook/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
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;
},
};
Comment on lines +1 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, can you try: grunt build --dist and then npm run storybook ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

it's weird. runs fine with npm start

Copy link
Contributor

Choose a reason for hiding this comment

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

@edsilv, did you mean grunt build --dist? I tried running npm build --dist and just got an error message.

Copy link
Member

Choose a reason for hiding this comment

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

npm run build --dist

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

(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?

2 changes: 2 additions & 0 deletions .storybook/preview-head.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
<script src="https://unpkg.com/[email protected]/dist/ResizeObserver.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

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...)

<script src="/uv-assets/js/bundle.js"></script>
22,348 changes: 15,413 additions & 6,935 deletions package-lock.json

Large diffs are not rendered by default.

11 changes: 10 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
"lint:all": "prettier --write \"./src/**/*.{js,jsx,json,css,ts,tsx}\" \"!./src/lib/* \"",
"postinstall": "opencollective postinstall",
"start": "npx serve www",
"test": "jest"
"storybook": "start-storybook -p 6006 -s ./dist",
"test": "jest",
"build-storybook": "build-storybook"
},
"repository": {
"type": "git",
Expand All @@ -36,9 +38,12 @@
},
"homepage": "https://github.com/universalviewer/universalviewer",
"devDependencies": {
"@babel/core": "^7.9.0",
"@storybook/html": "^5.3.18",
"@types/jest": "22.2.2",
"@types/puppeteer": "1.9.0",
"async": "0.9.0",
"babel-loader": "^8.1.0",
"chai": "4.1.2",
"chalk": "0.5.1",
"glob": "7.1.3",
Expand Down Expand Up @@ -84,6 +89,8 @@
"@iiif/iiif-tree-component": "^2.0.3",
"@iiif/manifold": "2.0.2",
"@iiif/vocabulary": "1.0.16",
"@storybook/addon-info": "^5.3.18",
"@storybook/addon-knobs": "^5.3.18",
"@types/modernizr": "3.2.29",
"@types/node": "8.10.52",
"@universalviewer/uv-cy-gb-theme": "1.1.2",
Expand All @@ -97,12 +104,14 @@
"jquery-ui-dist": "1.12.1",
"jquery-ui-touch-punch": "0.2.3",
"jsviews": "0.9.83",
"jsx-dom": "^6.4.14-beta.3",
"manifesto.js": "4.0.1",
"mediaelement": "4.2.15",
"opencollective": "1.0.3",
"openseadragon": "2.4.2",
"pdfjs-dist": "2.0.161",
"pdfobject": "2.1.1",
"react-docgen-typescript-loader": "^3.7.2",
"waveform-data": "2.0.2",
"xss": "1.0.3"
},
Expand Down
3 changes: 2 additions & 1 deletion src/Viewer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this the case?

Copy link
Member

Choose a reason for hiding this comment

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

I think this was related to a bug when accessing globals like $?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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");

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ill do that 👍


interface IExtensionLoaderCollection {
[key: string]: () => any;
Expand Down
4 changes: 2 additions & 2 deletions src/lib/bundle.js

Large diffs are not rendered by default.

50 changes: 50 additions & 0 deletions stories/uv.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/** @jsx h */
import { h } from 'jsx-dom';
import { withKnobs, text, number } from '@storybook/addon-knobs';
import { Viewer } from '../src/index';

export default { title: 'Universal Viewer', decorators: [withKnobs] };

type Data = {
manifestUri: string,
configUri?: string,
collectionIndex?: number,
manifestIndex?: number,
sequenceIndex?: number,
canvasIndex?: number,
rangeId?: number,
rotation?: number,
xywh?: string,
embedded?: boolean,
locales?: any, //??
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this ?? comment indicate that further work/discussion is needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, as far as I can tell, there are no types for the locale config in the UV.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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).

Copy link
Member

Choose a reason for hiding this comment

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

I think typing it shouldn't be a problem

};

const UVDemo = (data: Data) => {
const container = <div style={{ width: '100%', height: '100vh', minHeight: 500 }} /> as HTMLElement;

const uv = new Viewer({
target: container,
data
});

uv.on('created', () => {
uv.resize();
}, {});

return container;
};


export const ScottishBridges = () => {
const manifestUri = text('Manifest URI', 'https://view.nls.uk/manifest/7446/74464117/manifest.json');
const canvasIndex = number('Canvas Index', 30);

return <UVDemo manifestUri={manifestUri} canvasIndex={canvasIndex || 0} />
};

export const Wunder = () => {
const manifestUri = text('Manifest URI', 'https://wellcomelibrary.org/iiif/b18035723/manifest');
const canvasIndex = number('Canvas Index', 0);

return <UVDemo manifestUri={manifestUri} canvasIndex={canvasIndex || 0} />
}
8 changes: 7 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,15 @@
"noUnusedParameters": false,
"removeComments": false,
"strictNullChecks": true,
"jsx": "react",
"jsxFactory": "h",
"suppressImplicitAnyIndexErrors": true,
"target": "ES5",
"tsBuildInfoFile": "./buildcache/front-end",
"rootDirs": [
"stories",
"src"
],
"types": [
"@edsilv/jquery-plugins",
"@iiif/base-component",
Expand All @@ -32,4 +38,4 @@
"include": [
"src"
]
}
}