AI rubric product tour with intro js #57757

Nokondi · 2024-04-03T15:57:38Z

Installed intro.js and intro.js react wrapper
Setup steps and placement of product tour
Created new scss theme to style product tour.
[Bonus: this PR also resolves the issue in AITT-515

Recording.2024-04-03.105240.mp4

Links

Figma Mock
Jira Ticket

…nstall-intro-js

davidsbailey

Woohoo, so exciting to see this come together! Some initial comments are inline.

Top-level comment: the spec says "Tour should display the first time a user lands on a page with an editable rubric", but when I go to http://localhost-studio.code.org:3000/s/csd3-2023/lessons/11/levels/4 (as opposed to /5), I still see the tour. I think this looks like a bug -- it is confusing because while the tour works, the highlighted features then disappear since evaluation is not possible on that level.

davidsbailey · 2024-04-04T19:45:46Z

apps/src/templates/rubrics/RubricContainer.jsx

@@ -52,12 +57,15 @@ export default function RubricContainer({

  const [feedbackAdded, setFeedbackAdded] = useState(false);

+  const [stepsEnabled, setStepsEnabled] = useState(false);


rename to productTour? If these mean the same thing, I think it would be helpful for readability to name them consistently

davidsbailey · 2024-04-04T19:49:23Z

dashboard/config/routes.rb

@@ -1082,6 +1082,8 @@
        get 'get_teacher_evaluations_for_all'
        get 'ai_evaluation_status_for_user'
        get 'ai_evaluation_status_for_all'
+        get 'get_ai_rubrics_tour_seen'
+        get 'update_ai_rubrics_tour_seen'


this isn't safe -- please use PUT or POST for anything that can cause a state change.

davidsbailey · 2024-04-04T19:52:40Z

apps/src/templates/rubrics/RubricContainer.jsx

+  const showSettings =
+    onLevelForEvaluation && teacherHasEnabledAi && !stepsEnabled;
+
+  // Steps for product tour


please extract product tour config (here through line 231) to a separate JS file to cut down on file size

davidsbailey · 2024-04-04T19:54:55Z

apps/src/templates/rubrics/RubricContainer.jsx

+            {/* {showSettings && (
+              <RubricSettings
+                productTour={stepsEnabled}
+                visible={selectedTab === TAB_NAMES.SETTINGS}
+                refreshAiEvaluations={fetchAiEvaluations}
+                rubric={rubric}
+                sectionId={sectionId}
+                tabSelectCallback={tabSelectCallback}
+              />
+            )} */}


please re-enable or remove

this may be moot however if you are getting rid of the stepsEnabled ternary

davidsbailey · 2024-04-04T19:58:03Z

apps/src/templates/rubrics/introjs.scss

+.introjs-overlay {
+  position: absolute;
+  box-sizing: content-box;
+  z-index: 999999;


this seems like a very high starting point, and the size of the numbers makes it difficult to see how they relate to each other. If possible I would suggest using numbers at most in the 100-200 range for improved readability. if another high z-index prevents this... let's take a follow-up item to reduce the arms race here.

this issue looks like it could still use follow-up (in a later PR)

…nstall-intro-js

davidsbailey

sorry for splitting my comments up -- I've finished reviewing now. nice job pulling this together!

my other top-level feedback is that this should really have a UI test: https://codedotorg.atlassian.net/browse/AITT-575

davidsbailey · 2024-04-04T21:23:19Z

apps/src/templates/rubrics/RubricContainer.jsx

-              alt={i18n.rubricAiHeaderText()}
-            />
-            <span>{i18n.rubricAiHeaderText()}</span>
+      {stepsEnabled ? (


I am seeing a lot more duplicated code between the two branches of this conditional compared to the number of differences. my recommendation would be to DRY this up and use this conditional inline to change the behavior each place a difference is needed. otherwise it seems like we are setting ourselves up for the product tour to lag or break as other updates are made to the UI.

davidsbailey · 2024-04-04T21:23:41Z

apps/src/templates/rubrics/RubricContainer.jsx

+            {/* {showSettings && (
+              <RubricSettings
+                productTour={stepsEnabled}
+                visible={selectedTab === TAB_NAMES.SETTINGS}
+                refreshAiEvaluations={fetchAiEvaluations}
+                rubric={rubric}
+                sectionId={sectionId}
+                tabSelectCallback={tabSelectCallback}
+              />
+            )} */}


this may be moot however if you are getting rid of the stepsEnabled ternary

davidsbailey

juuust one more comment 😅

davidsbailey · 2024-04-04T21:48:29Z

apps/test/unit/templates/rubrics/RubricContainerTest.jsx

+
+    expect(screen.findAllByText('Getting Started with AI Teaching Assistant'))
+      .to.be.empty;
+  });


I'm glad to see you're leaning in to React Testing Library! Since it seems like things part way through the product tour could break, my recommendation would be to have some coverage of clicking through the tour. this seems important because people will inevitably be making changes without necessarily thinking about the tour, which risks breaking it.

davidsbailey · 2024-04-05T00:02:23Z

hey Mark, since the launch deadline is coming up, I want to say the min bar for shipping this is (1) turn the GET into a PUT/POST, and (2) make it not show up on non-eval levels, and (3) if possible a tiny bit more unit test coverage. I think the rest can be deferred to a follow-up PR, as long as it's done before moving onto other tasks.

…nstall-intro-js

davidsbailey

looks good to merge once tests are passing. once you're back next week, it would be great to make sure we have a list of any PR comments not addressed in order to hit the ship deadline (if any) so that we don't lose track of them.

davidsbailey · 2024-04-07T01:23:58Z

dashboard/test/ui/features/teacher_tools/rubrics/ai_evaluate_student_code.feature

@@ -39,6 +39,7 @@ Feature: Evaluate student code against rubrics using AI
    And element ".teacher-panel td:eq(1)" contains text "Aiden"
    And I click selector ".teacher-panel td:eq(1)" to load a new page
    And I wait for the page to fully load
+    And I click selector ".introjs-skipbutton" if it exists


from the saucelabs video, it looks like the skipbutton is not getting clicked.

looking at this test step, it looks unreliable because when the "I wait for the page to fully load" step completes, the skipbutton may not have rendered yet. this kind of timing issue can often explain the frustrating issue of not being able to repro locally.

to prove/disprove, you could add a wait here (10 sec) to see if that fixes.

for a shippable solution, you could do something like:

choose an element that will be in the DOM regardless of whether introjs is active, and render it differently when introjs is active

wait for that element to load

write a custom step that looks at that element and decides whether to wait for the skipbutton, and then click it

...at this point if the sleep statement fixes it, I think it could be ok to use that approach to get this PR unblocked, and do the "shippable" fix as a fast follow

davidsbailey

@Nokondi I think this is a great workaround for the unit test issues. once again nice job pulling this all together! Looks good to merge. I am seeing one follow-up remaining, for the high z-indexes.

davidsbailey · 2024-04-09T15:43:07Z

dashboard/test/ui/features/teacher_tools/rubrics/teacher_view_of_rubric.feature

+  And I wait until element "h1:contains(How did we do?)" is visible
+  And I wait until element ".introjs-tooltiptext" is visible
+  And I click selector ".introjs-button:contains(Done)" once I see it


to complete the scenario, I would suggest additional steps which verifies that we're now back into a mode where the teacher can interact with the rubric UI. For example:

verify contents of rubrics UI matches what it should look like for the current level (as opposed to the tour)

switch tabs or learning goals, and confirm that the UI updates correspondingly

since this has been tested manually, this seems fine to do as a fast-follow.

Ah, the high z-index is actually part of the introjs module. I had to override their built-in css for some of our styling, so some elements (like the ones with the crazy z-index) are copied straight from their css.

davidsbailey · 2024-04-09T15:45:32Z

apps/src/templates/rubrics/RubricContainer.jsx

+      .then(response => {
+        return response.json();
+      })


I like this as a way to make sure we do not read the response body/json twice

davidsbailey · 2024-04-09T15:46:57Z

apps/src/templates/rubrics/introjs.scss

+.introjs-overlay {
+  position: absolute;
+  box-sizing: content-box;
+  z-index: 999999;


this issue looks like it could still use follow-up (in a later PR)

davidsbailey · 2024-04-09T15:47:47Z

apps/src/templates/rubrics/productTourHelpers.js

davidsbailey · 2024-04-09T20:43:48Z

Ah, the high z-index is actually part of the introjs module. I had to override their built-in css for some of our styling, so some elements (like the ones with the crazy z-index) are copied straight from their css.

ugh! I see these now. boo z-index arms race. I agree in that case there is nothing to do about it then.

Nokondi added 17 commits March 28, 2024 12:21

install introjs

6eefc09

install introjs-react

ecdd085

Add product tour steps

d8f6e83

Product tour implemented

f0fc5ea

Style updates + evidence demo gif

017e8f4

add serialized attr

5e0d252

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into i…

befa66a

…nstall-intro-js

style updates

666afe7

New tour styling setup

d007c48

style updates

45afbd2

style updates

8038015

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into i…

e975023

…nstall-intro-js

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into i…

5af5389

…nstall-intro-js

adding evidence to dummy data

3e1e777

removed stray routes

b6bdf3c

Additional requested changes

2db44f7

unit tests

b422c7e

Nokondi requested review from a team, etaderhold and davidsbailey and removed request for a team and etaderhold April 4, 2024 17:19

Nokondi marked this pull request as ready for review April 4, 2024 17:20

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into i…

7d3464f

…nstall-intro-js

davidsbailey reviewed Apr 4, 2024

View reviewed changes

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into i…

c2aaa94

…nstall-intro-js

davidsbailey reviewed Apr 4, 2024

View reviewed changes

unit test updates

404c970

Nokondi added 6 commits April 5, 2024 14:56

changed get to post

7daf177

unit test updates

2e30cef

unit test updates

5f33333

Merge branch 'staging' of github.com:code-dot-org/code-dot-org into i…

9c13c28

…nstall-intro-js

Quick fix

7c3a396

unit tests

9e38a4b

davidsbailey approved these changes Apr 5, 2024

View reviewed changes

Nokondi added 5 commits April 6, 2024 10:53

unit tests

ff9529d

unit tests

f1ad0f9

update account steps

350f9ed

test updates

4472c60

test updates

440defa

Nokondi requested a review from a team as a code owner April 6, 2024 22:43

Nokondi added 3 commits April 6, 2024 19:00

UI test updates

c8c7110

Unit test update

63f0394

more tests

50746d3

davidsbailey reviewed Apr 7, 2024

View reviewed changes

Nokondi added 5 commits April 6, 2024 22:57

more unit test changes

43e334f

updated regex

d238947

another unit test update

378dea1

added timer to tour test

57f98bd

Added UI test for product tour

42a932a

davidsbailey reviewed Apr 9, 2024

View reviewed changes

Nokondi added 3 commits April 9, 2024 13:04

bug fixes

795d66b

removed dead code

b348294

ui test fix

12edf0b

ui test fix

900ca8d

Nokondi merged commit 41abb67 into staging Apr 9, 2024
1 of 2 checks passed

Nokondi deleted the install-intro-js branch April 9, 2024 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AI rubric product tour with intro js #57757

AI rubric product tour with intro js #57757

Nokondi commented Apr 3, 2024

davidsbailey left a comment

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey Apr 9, 2024

davidsbailey left a comment

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey Apr 4, 2024

davidsbailey left a comment

davidsbailey Apr 4, 2024

davidsbailey commented Apr 5, 2024

davidsbailey left a comment

davidsbailey Apr 7, 2024

davidsbailey Apr 7, 2024

davidsbailey left a comment

davidsbailey Apr 9, 2024

Nokondi Apr 9, 2024

davidsbailey Apr 9, 2024

davidsbailey Apr 9, 2024

davidsbailey Apr 9, 2024

davidsbailey commented Apr 9, 2024

		@@ -52,12 +57,15 @@ export default function RubricContainer({

		const [feedbackAdded, setFeedbackAdded] = useState(false);

		const [stepsEnabled, setStepsEnabled] = useState(false);

AI rubric product tour with intro js #57757

AI rubric product tour with intro js #57757

Conversation

Nokondi commented Apr 3, 2024

Links

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey commented Apr 5, 2024

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey commented Apr 9, 2024