Handle ?tutorial= query in gui #3725
Conversation
|
Hm, seems unfortunate that Is it possible to detect the presence of a tutorial in the querystring and hide the modal, as we do in |
|
|
benjiwheeler
left a comment
benjiwheeler
left a comment
There was a problem hiding this comment.
This is exciting, and I think we might need to talk about how to handle tutorial URLs more broadly... does the presence of a tutorial querystring mean we are currently showing the tutorial? Or, that we start showing it? Do we have to strip it from the URL when the user closes the tutorial?
| getIsShowingProject | ||
| } from '../reducers/project-state'; | ||
| import {setProjectTitle} from '../reducers/project-title'; | ||
| import {detectTutorialId} from '../lib/tutorial-from-url'; |
There was a problem hiding this comment.
is it possible to move this url detection to an HOC? Seems like that's the design pattern we're trying to follow, and moving towards.
I think we could do the same componentDidMount logic in the HOC, and it should just work, because gui watches the reducer values already.
What do you think? Partially, I'm thinking forward to our needing to mess with the url even more... seems like an HOC wrapping the whole gui at the top level would be good.
| // handle ?tutorial=all for beta | ||
| // if we decide to keep this for www, functionality should move to | ||
| // setActiveCards in the GUI container | ||
| if (tutorialId === 'all') initializedGui = initTutorialLibrary(initializedGui); |
There was a problem hiding this comment.
This should still disable the preview modal if there's a tutorialId @benjiwheeler
|
The project id is not automatic. The link from code.org has the project id in the URL. To see it you'd need to add #249143200 to the url. Not suppressing the try-it modal is a problem - I'll fix it. |
chrisgarrity
force-pushed
the
issue/www-2281-tutorials
branch
from
6a9c191 to
719aaf6
Compare
|
Thanks, I was able to bring up the project with id and tutorial. But tests are failing... |
benjiwheeler
added
pr - changes requested
and removed
needs discussion
pr - needs review
labels
Moved the tutorial id handling into gui so it works when gui is embedded in www as well as standalone. This seemed better than adding the same query handling to www, in order to pass an id to gui. Note, the `?tutorial=all` handling remains in app-state-hoc. Currently it’s only needed in standalone mode for Hour of Code. It’s easy to move into the gui container if we do want it in gui by default.
chrisgarrity
force-pushed
the
issue/www-2281-tutorials
branch
from
719aaf6 to
b287cf6
Compare
Also pull out `onUpdateReduxDeck` prop out to avoid warning when it’s passed to nested components.
chrisgarrity
force-pushed
the
issue/www-2281-tutorials
branch
from
b287cf6 to
3efab34
Compare
2 participants
Resolves
Prerequisite for scratchfoundation/scratch-www#2281
Proposed Changes
Moved the tutorial id handling into gui so it works when gui is embedded in www as well as standalone.
This seemed better than duplicating the query handling in www, in order to pass an id to gui.
Note, the
?tutorial=allhandling remains in app-state-hoc. Currently it’s only needed in standalone mode for Hour of Code. It’s easy to move into the gui container if we do want it in gui by default.Question: are there going to be interactions with project save/new... that will lead to ignoring the query string, or otherwise not the results that users expect.
Reason for Changes
Support opening with a tutorial loaded in www.
Test Coverage
Current tests pass - it didn't change functionality, just where it was included so it's available in www.
Try it:
https://chrisgarrity.github.io/scratch-gui/issue/www-2281-tutorials/
Manual testing:
/?tutorial=getStartedshould open the getting started tutorial/?tutorial=allshould load with the tutorials library openBrowser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet