AiLab: Dynamic Instructions get overlay & Markdown #40469
Conversation
breville
changed the title
Conflicts: apps/src/ailab/Ailab.js
breville
requested review from
Erin007,
made-line,
dmcavoy,
maureensturgeon and
tess323
|
My goodness this is cool! I am ooo until Friday afternoon so don’t want to hold this up. We’re using this component in lesson plans (will be live 5/25) so would like to have @bethanyaconnor take a look to see if we’re expecting disruption. I can test closer on staging. |
Pulled this locally and did a quick check and it looks good to me. The change to the component we use in lesson plans is small. It looks like we don't display the write instructions for this type of level so I created https://codedotorg.atlassian.net/browse/PLAT-1045 to track that. Small thing when I was testing -- looking like the instructions cover up the tooltip for the bubbles |
| @@ -556,7 +556,8 @@ class TopInstructions extends Component { | |||
| isRtl ? styles.mainRtl : styles.main, | |||
| mainStyle, | |||
| { | |||
| height: height - RESIZER_HEIGHT | |||
| height: height - RESIZER_HEIGHT, | |||
| zIndex: 1021 | |||
There was a problem hiding this comment.
I'm curious, is this an arbitrary z index? Do we have guidelines on what values to use or what the boundaries of z indexes are?
There was a problem hiding this comment.
I've updated this to show where it comes from (borrowed from InstructionsCSF) and only set it when the overlay is showing beneath a dynamic instruction, which prevents the broader regression that @bethanyaconnor caught. I'm not familiar with central guidelines, but there are occasional comments such as here. Thanks go to you and @bethanyaconnor for the great catches.
apps/src/ailab/Ailab.js
Outdated
| results: 'Review the results.', | ||
| saveModel: 'Save the trained model for use in App Lab.', | ||
| trainingSettings: 'Adjust column types here if you want.', |
There was a problem hiding this comment.
Can we track or add a comment about removing this line when main-next is merged since the training settings screen no longer exists? (code-dot-org/ml-playground#178)
There was a problem hiding this comment.
Good point. Given that we're about to reach that point, I just removed the unnecessary strings.
There was a problem hiding this comment.
Couple tiny tidying requests, but looks good! This does a great job of drawing attention to the instructions. I want to make sure that the curriculum team is on board with the typing animation - it's great for a short line of text, but know they often have longer blocks of instructions and want markdown support to do more complicated things in the instructions panel so wonder how the animation would feel in those scenarios. If you've got a 👍 from them, then I don't have any concerns about the code.
breville
changed the title
|
@erin Good point. I had intended this to be a quick stopgap measure before circling back to Markdown, but given our current point in the schedule, I just went ahead and switched over to Markdown now. |
Dynamic Instructions now support optionally showing the overlay when the instructions are set or changed, drawing more attention to the new instructions. And they now use Markdown.
See code-dot-org/ml-playground#177 for AI Lab's usage.
Note that the Markdown usage is currently intended for text. It's unlikely that images will currently work properly, because they are loaded asynchronously by the browser, but the Dynamic Instructions system needs to measure the rendered Markdown immediately to determine the maximum size of the instructions. (Curriculum is okay with this limitation for AI Lab.)
AI Lab is also notified when the overlay is dismissed, allowing it to do things like start animations, as can be seen in the training scene in the below video.
dynamic-instructions-overlay-2.mov