Feature/CSL-2007/CSL-2040 group 1 and 2 enhanced tooltips #328

klown · 2022-10-14T21:18:44Z

Create the "group 2" popover dialogs and define their targets such that they show when the relevant page is loaded, and the criteria for showing them is met.

Note that since this PR is an extension of the group 1 popovers, it is based on and includes the changes in PR #323. Going forward this PR will track changes for both group 1 and group 2 enhanced tooltips.

JIRAs:

- add content for 'view' and 'readaloud' popovers - switch to popover styles and classes

…ltips

Converted tooltips to popovers for settings, wordbank, switch, context, book_actions, and activity.

… start of chainable tour

The `kind` field denotes whether the tip is a popup tooltip or a dialog popover. The latter are associated with "robust tooltips" and tours.

- the `kind` of tip -- tooltip vs dialog -- is not used anywhere; remove for now. - fix the name of the settings teacher resource to link to in the `popover_tour.html` template.

The migrations cancel each other. The first adds a `kind` field to the `TipType` model while the second removes it.

… if only one in chain

…S. Stopgap for refocus of control

…shown

…is shown

…how()`

…e/CSL-2040-group-2-enhanced-tooltips

Added: - reading_details (Active Reading time and Details), - reading_data (Student reading data), - cluey, but only partially (Cluey), - thoughts (Your thoughts). With respect to Cluey, the popover dialog is ready, but there is no target element to attach the popover to. See comment in CSL-2040: https://castudl.atlassian.net/browse/CSL-2040?focusedCommentId=36759

Also, remove debugging `pdb` statements.

klown · 2022-10-18T14:57:09Z

@bgoldowsky , while I've marked this as "ready for review", there are changes to both the kind of tip for Cluey, and its content. It looks like it will revert to an actual tooltip that is shown on mouse hover, and not a spontaneous display that depends on the history of when it was last shown. See Kristen's comment in the JIRA.

Given that change, I need to at least change the logic for showing Cluey's popover, likely remove it from that logic.

klown · 2022-10-18T22:51:08Z

Based on a conversation with Kristen in the JIRA for CSL-2007, the group 2 Active Reading time and Details is a duplicate of the group 1 Tooltip_student activity. The latter is a replacement for an existing tooltip and implemented in PR #323. The former was implemented in this PR as a new popover. I will remove the new popover from here. This entails removing a TipType from the database and adjusting priorities as appropriate.

The conversation in the JIRA starts with a note that the group 1 popover is missing from the Dashboard tour.

…actions

…ated actions

…ned" and "Popular" tabs on the teacher Dashboard

…tribute

… Reading pages.

…the `student_reactions` tip type

…g_data` tip type

…f the book

klown · 2022-10-28T14:39:09Z

@bgoldowsky @mbrambilla This is ready for review. All the group 1 and group 2 "enhanced tooltips" are in place, including the recent two for the student Dashboard.

The last code I added was a couple of unit tests for changes I made to TipType.can_show(). Good thing, since I found and fixed bug in my changes. I'm inclined to add more automated tests for the logic underlying what/if a popover should show. What do you think?

As we discussed at the technical meeting, this PR is ready to merge @mbrambilla's code from PR #333 into it. Alternatively, this could be reviewed and merged for Q/A while the tour work continues on its branch. Let me know how you think we should proceed.

…tamp When `register_show()` is called with a timestamp that is earlier than the current history's `last_show`, the value of `last_show` should not change.

The relevance of a tip history's `last_show` time's seconds and microseconds are minor. and are excluded from one test.

bgoldowsky · 2022-11-01T17:49:49Z

src/library/views.py

        context = super().get_context_data(**kwargs)
        context['search_form'] = self.search_form
-        context['tip_name'] = self.tip_shown.name if self.tip_shown else None
+        # BEGIN: Sample Tour


Is this a sample, as opposed to something that should remain for production use?

This is kind of a hybrid mode between the tooltips (tip_name) and the popovers (tours). This has been cleaned up in PR #336. I was originally planning on add the tour chains into the original PR, but things forked in two different directions into this PR and then the tour PR.

bgoldowsky · 2022-11-01T17:57:16Z

src/pages/views.py

-        context['tip_name'] = self.tip_shown.name if self.tip_shown else None
+        context['clusive_user'] = self.clusive_user
+        # BEGIN: Sample Tour
+        # Sample tour with single item list


Transitional hybrid mode resulting from trying to do too much in one PR, cleaned up in PR #336.

bgoldowsky · 2022-11-01T17:57:35Z

src/pages/views.py

            'cuelist': json.dumps(cuelist),
            'hide_cues': hide_cues,
-            'tip_name': self.tip_shown.name if self.tip_shown else None,
+            # BEGIN: Sample Tour


Transitional hybrid mode resulting from trying to do too much in one PR, cleaned up in PR #336.

What are the implications for this PR, then? Is it simply a matter of removing the "sample tour" comments? It goes back to the issue of whether the singleton popovers can be merged vs. combining this branch with the tour branch.

bgoldowsky · 2022-11-01T18:46:43Z

src/tips/models.py

        """Test whether this tip can be shown on a particular page"""
        # Teacher/parent-only tips
-        if user.role == Roles.STUDENT and self.name in ['book_actions', 'activity']:
+        if user.role == Roles.STUDENT and self.name in TEACHER_ONLY_TIPS:


Also Roles.GUEST or use something like "not user.can_manage_periods". Currently guests are being assigned teacher-only tips.

I should have included a GUEST user in the tests. When GUEST is tested, the TipType.can_show() fails for the activity tip on the Dashboard.

By implication, teacher tips should also show for guests using the current version in production where only book_actions and activity tips are checked in TipType.can_show(). Do book_actions or activity tooltip pop-up for guests? I suspect not because the Dashboard for guests does no include the relevant activity panel nor the book_actions menu. As such, there is nothing for the tooltip to point to.

But, in the current situation, there are more TEACHER_ONLY_TIPS some of whose targets appear for guest users. So, I'll check for guest users.

bgoldowsky · 2022-11-01T18:47:03Z

src/tips/models.py

        if self.name == 'switch':
            return page == 'Reading' and version_count > 1
+        # Thoughts TipType is only for students
+        if self.name == 'thoughts' and user.role != Roles.STUDENT:


Also GUEST, see above.

bgoldowsky · 2022-11-01T19:10:15Z

src/shared/templates/shared/partial/popover_tour.html

+        </div>
+        {% elif tour_name == "book_actions" %}
+<!-- book_actions -->
+        {% if clusive_user.role == 'TE' %}


This file with all of its 'if' statements feels extremely fragile, it would be great to do some refactoring. Why is this one just TE, while most are TE or PA? Is there some reason that parents should not get to see the video?

Yeah, I'll see if I can refactor and simplify the template. As for the missing "or PA", that's something like a copy-paste error.

@mbrambilla what kind of grief am I going to cause you due to a refactoring of the popover_tour.html template?

@klown Most of my refactoring in popover_tour.html was around the 'footer' section, as seen here - you will need to click the 'load diff' link to see the changes: #336 - popover_tour.html

The goal was to create a re-usable template for the footer section to reduce redundant template markup.

Thanks @mbrambilla. I thought about merging just your footer change into this PR, but it has features that don't apply here or might break things. I'll work on making a suitable version. Since that is where most of your refactoring was done, it should make merge conflicts smaller.
🤞

bgoldowsky · 2022-11-01T19:12:26Z

src/shared/templates/shared/partial/popover_tour.html

+        <p>Click “More actions” (three dots) to assign or customize any book, and for books you uploaded to Clusive, to edit information or delete a book.</p>
+        <div class="row gx-0_5 flex-items-center">
+            <div class="col">
+                {% if has_teacher_resource %}


What is the purpose of passing in the 'has_teacher_resource' variable, given that this template has hard-coded the URL of any teacher resource that exists already? In this case, has_teacher_resource is passed in as false by LibraryView. Clearly there is one, but the link is not displayed.

The general rationale of has_teacher_resource was flexibility in showing the link. It should have been named has_teacher_resource_link or, even better, show_teacher_resource_link.

Admittedly it's somewhat muddled in that the link is not shown for student and guest users, somthing that could be handled by show_teacher_resource_link. But the template covers that situation by the check for a TE or PA role*.

Still, there are cases where the user is a teacher or a parent and the link should not be shown. For example, if the user has navigated to the Manage page and the Manage popover shows, the link in the popover is odd since it just navigates to the page the user is viewing. It should not be shown in this case.

As for the Library case, that's a bug; it should not be false.

*Perhaps this is part of the refactoring: Move the role check logic to views.py to set show_teacher_resource_link and then use only that boolean in the template. It would remove some clutter.

bgoldowsky · 2022-11-01T19:14:26Z

The PR is a little confusing because it contains things related to the tour, but doesn't actually implement the tour. So I think things like the 'tour samples' passed in context variables are harmless, but hope they will be cleaned up in the next PR.

There are defintely some logic errors, mostly due to checking for STUDENT or TEACHER but forgetting about GUEST and PARENT.

…show

…ow()`

Some of the logic for showing the "Learn more" link is in the popover dialog footer template -- if the page requested and the "Learn more" link are the same, do not show the link.

klown · 2022-11-07T15:03:37Z

@boris, @mbrambrilla, I have made changes in response to the review comments, mainly to refactor and reorganize the popover_tour.html template. I think it could be simplified further if some context such as the title of the popover and the video link were set in the views.py. But I'm not sure what belongs in the template and what belongs in the code; and am leaning towards the titles being in the template. On the other hand, if they and other context were in the code, then the large {% if %} could be eliminated. Or, perhaps just moved into the views.py code.

{% if tour_name == ...%} 
... 
{% elif tour_name == ... %}
...
{ % elif ... %}
...
{% endif %}

In addition, I noticed another page that was not included in the show-tip logic. The page is the ResourcePage. The result is that when using the Reader to view a teacher resource, there are no popovers, and, by implication, there will be no tour. I modified the code to treat the ResourcePage as having the same tip types as the Reading page. Does that make sense?

klown · 2022-11-07T18:56:33Z

Regarding more refactoring, I think there is a structure for each popover along the following lines:

tour_properties: {
    'settings': {
        'title': 'Settings',
        'video': 'https://videos.sproutvideo.com/embed/a79edbb41414e7c72e/96f3dfacf85e2603?type=hd',
        'tip_text': '<p>Change the font, colors, and more! Settings let you choose the look, supports, and reading tools to make Clusive all your own.</p>',
        'resource_id': 'SSETTINGS',
        'resource_name': 'settings',
    },
    'readaloud': {
        'title': 'Read-aloud',
        'video': 'https://videos.sproutvideo.com/embed/449edbb51c1ae6cecd/90717ea3b61bbdd3?type=hd',
        'tip_text': '<p>Click the page read aloud icon to hear the entire page. Select text and click read aloud in the context menu to hear a smaller selection or a single word read aloud.</p>',
        'resource_id': None,
        'resource_name': 'None,
    },
    ...
}

It's a lookup-table where each tip type (e.g., settings) defines a set of values for that popover dialog, namely, its title, the link to its video, the tip text, and its teacher resource link, if any. The properties appropriate to the response are keyed by the tip_shown in the context and are set as tour_properties.

In that case, I think the template's {%if tour_name == ...%) ... {% elif %} ... {% endif %} is reduced to just one template where the tour_properties fields fill at the appropriate places.

klown · 2022-11-08T14:42:22Z

I wrote:

Regarding more refactoring, I think there is a structure for each popover along the following lines:

I pushed a version of this approach to another branch based on this one. This shows the markup for all but one of the popover dialogs. The context popover turns out to be a special case.

Allow the test to pass if the actual time is within a minute of the expected time since, in reality, the actual time is later than the time passed in when set.

klown and others added 21 commits October 4, 2022 10:41

CSL-2007: Migrate tooltips to popover dialogs (first steps)

3dc4479

- add content for 'view' and 'readaloud' popovers - switch to popover styles and classes

CSL-2007: Append "video" to the video iframe's aria-label

a0ae9c3

Merge branch 'development' into feature/CSL-2007-group1-enchanced-too…

d15476f

…ltips

CSL-2007: Convert to popover dialogs for the remaining tips

b780de7

Converted tooltips to popovers for settings, wordbank, switch, context, book_actions, and activity.

CSL-2007: Split apart tooltip and popover template variants, stub out…

5c4f19b

… start of chainable tour

CSL-2007: Fix typo property to variable

2dbb86b

CSL-2007: Add kind field to TipType model

358066f

The `kind` field denotes whether the tip is a popup tooltip or a dialog popover. The latter are associated with "robust tooltips" and tours.

CSL-2007: Remove kind field from TipType model and fix tour template

e231b52

- the `kind` of tip -- tooltip vs dialog -- is not used anywhere; remove for now. - fix the name of the settings teacher resource to link to in the `popover_tour.html` template.

CSL-2007: Remove migration files as no longer needed

1e1b2d5

The migrations cancel each other. The first adds a `kind` field to the `TipType` model while the second removes it.

CSL-2007: Fix: remove kind from display version of TipType instances

b7dcca1

CSL-2007: Fix tts issue and settings markup merge error

2f8b289

CSL-2007- Fix context tip not showing, force focus to popover on show…

64a8f41

… if only one in chain

CSL-2007: Fix positioning issue with read aloud popover when using TT…

6c332f9

…S. Stopgap for refocus of control

CSL-2007: Fix name of settings resource url, SSETTINGS

a596881

CSL-2007: Remove empty "Learn more" links from the TAB order

5b98185

CSL-2007: Set focus on the title (header) of the popover dialog when …

aaadf61

…shown

CSL-2007: Revert focus to the .popover element when popover dialog …

c36dde9

…is shown

CSL-2040: Add new tip types to fixtures and to logic of TipType.can_s…

a85adc2

…how()`

Merge branch 'feature/CSL-2007-group1-enchanced-tooltips' into featur…

4671445

…e/CSL-2040-group-2-enhanced-tooltips

CSL-2040: Add "student reactions" to popover dialog template

68b3cc8

klown changed the title ~~Feature/csl 2040 group 2 enhanced tooltips~~ Feature/CSL-2040 group 2 enhanced tooltips Oct 14, 2022

klown added 4 commits October 17, 2022 11:25

CSL-2040: Add group 2 "Manage" popover dialog to the Dashboard

cd6d420

CSL-2040: Add group 2 "Resources" popover dialog to the Dashboard

3208afc

CSL-2040: Add group 2 "Filters" popover dialog to the Library page

dbaca5e

CSL-2040: Add group 2 "Search" popover dialog to the Library page

e587e51

Also, remove debugging `pdb` statements.

klown marked this pull request as ready for review October 18, 2022 14:51

klown mentioned this pull request Oct 19, 2022

Feature/CSL-2007: group1 enchanced tooltips #323

Closed

klown added 10 commits October 25, 2022 16:57

CSL-2040: Add triggers for reading_data tip histories' tip related …

0a6a8fa

…actions

CSL-2040: Add triggers for student_reactions tip histories' tip rel…

cf4af32

…ated actions

CSL-2040: Add trigger for reading_data tip related action to "Assig…

83672ae

…ned" and "Popular" tabs on the teacher Dashboard

CSL-2040: Fix misspelling of data-clusive-tip-action attribute

359986b

CSL-2040: Fix another spelling error re: data-clusive-tip-action at…

7c05e08

…tribute

CSL-2040: Implement thoughts popover for both student Dashboard and…

a231ec4

… Reading pages.

CSL-2040: Implement "Your thoughts" popover for student Dashboard as …

4365e53

…the `student_reactions` tip type

CSL-2040: Add "My reading" popover for student Dashboard as a `readin…

f3a00de

…g_data` tip type

CSL-2040: Add unit test for TipType.can_shown()

b161396

CSL-2040: Add test case for switch when there is only one version o…

b0f2196

…f the book

mbrambilla mentioned this pull request Oct 28, 2022

CSL-2041: Popover tours #336

Merged

klown added 6 commits October 31, 2022 12:24

CSL-2040: Added test for TipHistory.register_show()

2850913

CSL-2040: Fix typo in test case

fbc8353

CSL-2040: Add test for TipHistory.register_show() for earlier times…

ed2ad49

…tamp When `register_show()` is called with a timestamp that is earlier than the current history's `last_show`, the value of `last_show` should not change.

CSL-2040: Change test for TipHistory.register_show() to ignore seconds

802a290

The relevance of a tip history's `last_show` time's seconds and microseconds are minor. and are excluded from one test.

CSL-2040: Add test for TipHistory.register_action()

485386d

CSL-2040: Test for TipHistory.ready_to_show()

615a82e

bgoldowsky reviewed Nov 1, 2022

View reviewed changes

klown added 5 commits November 2, 2022 12:38

CSL-2040: Fix to include guest users when checking if a tip type can …

20565fc

…show

CSL-2040: Refactor popover tour templates

f6a0661

CSL-2040: Take ResourceReading page into account in `TipType.can_sh…

c058275

…ow()`

CSL-2040: Fix check for the resources tip type on the "Resources" page

823364f

CSL-2040: Adjust logic to determine if a "Learn more" link is shown

6109b87

Some of the logic for showing the "Learn more" link is in the popover dialog footer template -- if the page requested and the "Learn more" link are the same, do not show the link.

CSL-2040: Fix test comparing actual/expected last_show times

d733646

Allow the test to pass if the actual time is within a minute of the expected time since, in reality, the actual time is later than the time passed in when set.

bgoldowsky merged commit b7396dc into development Nov 9, 2022

Feature/CSL-2007/CSL-2040 group 1 and 2 enhanced tooltips #328

Feature/CSL-2007/CSL-2040 group 1 and 2 enhanced tooltips #328

Uh oh!

Conversation

klown commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klown commented Oct 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klown commented Oct 18, 2022

Uh oh!

klown commented Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klown Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrambilla Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klown Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klown Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgoldowsky commented Nov 1, 2022

Uh oh!

klown commented Nov 7, 2022

Uh oh!

klown commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klown commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

klown commented Oct 14, 2022 •

edited

Loading

klown commented Oct 18, 2022 •

edited

Loading

klown commented Oct 28, 2022 •

edited

Loading

klown Nov 2, 2022 •

edited

Loading

mbrambilla Nov 2, 2022 •

edited

Loading

klown Nov 2, 2022 •

edited

Loading

klown Nov 2, 2022 •

edited

Loading

klown commented Nov 7, 2022 •

edited

Loading

klown commented Nov 8, 2022 •

edited

Loading