Project template workspace icon #18570
Conversation
| @@ -498,6 +496,22 @@ StudioApp.prototype.init = function (config) { | |||
| this.emit('afterInit'); | |||
| }; | |||
|
|
|||
| StudioApp.prototype.initProjectTemplateWorkspaceIconCallout = function () { | |||
There was a problem hiding this comment.
I would push all of this down into the ProjectTemplateWorkspaceIcon component, in a componentDidMount lifecycle method.
Pro: This logic gets colocated with the component it has to attach to, and can safely run any time that component renders so there's no duplicated check for whether we are showing the icon. Also minimizes how much we're adding to StudioApp.js.
Con: Adds an external dependency and side-effect to what is currently a pure display component.
Thoughts?
There was a problem hiding this comment.
I can see both perspectives, but given that I'm needing to do special handling of this for weblab (see discussion below), I think it is best to leave it as is.
| }, | ||
| }; | ||
|
|
||
| export default class ProjectTemplateWorkspaceIcon extends React.Component { |
There was a problem hiding this comment.
👍 Thanks for using an ES6 class!
| export default class ProjectTemplateWorkspaceIcon extends React.Component { | ||
|
|
||
| render() { | ||
| const tooltipId = _.uniqueId(); |
There was a problem hiding this comment.
I'm not sure it matters, but I wonder if it's more correct to create a unique ID when the component is constructed, instead of every time it's rendered?
constructor(props) {
super(props);
this.tooltipId = _.uniqueId();
}There was a problem hiding this comment.
I copied how TooltipWithIcon is doing it. That does seem like a reasonable idea though. I'll try it out...
| > | ||
| <img | ||
| style={styles.projectTemplateIcon} | ||
| id={'projectTemplateWorkspaceIcon'} |
There was a problem hiding this comment.
Could just be a string attribute.
id="projectTemplateWorkspaceIcon"| // if we don't delay this slightly (currently 0.1 seconds) | ||
| setTimeout(() => { | ||
| this.studioApp_.initProjectTemplateWorkspaceIconCallout(); | ||
| }, 100); |
There was a problem hiding this comment.
This is interesting. Any idea what changes in those 100ms? If this is tied to bramble load time I'm worried that 100ms won't be enough on slow connections, or for users that haven't cached any of that content yet. I'm also curious if this is still a problem if we tie this call to the componentDidMount event (I kind of assume it would be, since it's still finding the DOM in the earlier call).
There was a problem hiding this comment.
It is already in the onMount of the WebLabView component. I tried for a while to figure out what was going on. I had a fix that used our "snap callouts" function that is used in droplet/blockly quite a bit (that basically fixes up the positions). But it looked strange to make it appear and then move it. There is something happening asynchronously in the React world (I believe) after the componentDidMount is complete. I tried hooking something to componentDidUpdate, but that doesn't get called at all for the initial load.
I'm open to suggestions, but figure that for a callout only on weblab, it probably wasn't worth going through a deeper multi-hour investigation. And I sort of like having a delay where the page loads and then the callout pops. Increasing the delay would be fine for the user experience in this case, but I can't prove what delay in "enough" since I don't really understand what is changing during that brief period of time...
There was a problem hiding this comment.
Sounds good. I wish we had a better understanding of this, but it doesn't need to block anything. 👍
| @@ -6,7 +6,6 @@ Background: | |||
| Given I am on "http://studio.code.org/s/algebra/stage/7/puzzle/4?noautoplay=true" | |||
| And I rotate to landscape | |||
| And I wait for the page to fully load | |||
| And I close the React alert | |||
There was a problem hiding this comment.
Are the uses of this step in contractEdtior.feature referring to a different alert?
If not, can we remove those and the related step as well?
Removed the project template workspace alert that previously appeared
Modified UI tests that expected the old alert to pop up and tried to dismiss it
Added a new icon that appears in the code workspace header to indicate that this is a project template workspace level with explanatory tooltip
Added some new UI tests to verify that the new icon appears when it should and does not appear when it should not
Added a callout that will appear once above that same icon so that users will more clearly see this icon the first time they encounter it (note that there is a single callout ID shared across all levels/scripts in code studio, so they won't see this over and over again)