-
Notifications
You must be signed in to change notification settings - Fork 67
Add story maps #994
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
base: main
Are you sure you want to change the base?
Add story maps #994
Conversation
|
Integration tests report: appsharing.space |
|
Demo looks great! I would love to have more time to help with a code review... sprinting for a workshop at a huge conference coming up. :( From the demo, the transition speed is really fast. What do you think of reducing it so that users have a better sense of where they are moving? Eventually, I'd love to have that fully configurable by the user, but that feels like a future PR :) |
|
CC @mfisher87 I added some options to change the animation duration and style, not sure about 'smooth' as a name though animations.mp4 |
|
Nice! Thanks, Greg! For the future, what do you think of being able to set transition settings for each individual landmark? |
Yes! I like this idea |
6b6d01e to
58284c1
Compare
|
bot please update snapshots |
martinRenou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Some notes:
- Can you add a story maps example to the list of examples?
- I got really confused because I created markers and not "landmarks", so I couldn't start my story. I wonder if it would make sense to combine the two? @gjmooney @mfisher87 thoughts?
A bug I noticed, if the image fails to load, the UI is messed up:
ui-tests/tests/notebook.spec.ts-snapshots/light-Notebook-ipynb-cell-2-chromium-linux.png
Show resolved
Hide resolved
ui-tests/tests/geojson-layers.spec.ts-snapshots/geoJSON-layer-chromium-linux.png
Show resolved
Hide resolved
| "storyMapPresentation": { | ||
| "type": "boolean", | ||
| "title": "Enable Story Map Presentations", | ||
| "default": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to make this disabled by default?
Maybe we should enable it by default and change the config to match other ones
| "storyMapPresentation": { | |
| "type": "boolean", | |
| "title": "Enable Story Map Presentations", | |
| "default": false | |
| "storyMapPresentationDisabled": { | |
| "type": "boolean", | |
| "title": "Disable Story Map Presentations", | |
| "default": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I went ahead and made this change because it's not that big of a deal, but considering that presentation mode is going to be off pretty much all of the time in most use cases, having it be on by default seems backwards.
packages/base/src/formbuilder/objectform/components/LandmarkReset.tsx
Outdated
Show resolved
Hide resolved
packages/base/src/panelview/components/story-maps/StoryViewerPanel.tsx
Outdated
Show resolved
Hide resolved
6acdad9 to
8a9ef1e
Compare
|
please update snapshots |
6c96289 to
e272afa
Compare
|
@gjmooney the snapshots issues should be fixed on main. Would you be able to rebase and fix the conflicts? |
e7d7eb5 to
c5b83be
Compare
|
bot please update snapshots |
|
bot please update snapshots |
9bedbca to
b634016
Compare
1fd447e to
7bb5d8c
Compare
Description
Adds story maps.
I wrote an updated description but github ate it so you just get videos.
storysofar2.mp4
Presentation mode:
presentation.mp4
Checklist
Resolves #XXX.Failing lint checks can be resolved with:
pre-commit run --all-filesjlpm run lint📚 Documentation preview: https://jupytergis--994.org.readthedocs.build/en/994/
💡 JupyterLite preview: https://jupytergis--994.org.readthedocs.build/en/994/lite