Add Studio/Insights links to Staff Instructor Toolbar by stvstnfrd · Pull Request #24020 · openedx/openedx-platform

stvstnfrd · 2020-05-19T00:01:21Z

Backport the new behavior from the Learning MFE.

Since this toolbar can appear outside of the unit-aware (and even courseware) pages, the link behavior is as follows:

Both Buttons

If the relevant Django settings aren't configured (not the case for us, but eg: some installs don't run Insights), CMS_BASE, ANALYTICS_DASHBOARD_URL, then no button is shown.

Insights Button

If you're on any course-aware page (including unit-aware),
- link goes to that course's Insights page.
If you're not on a course-aware page,
- link goes to main Insights dashboard.

Studio Button

If you're on any unit-aware page,
- link goes to that unit's Stdio page (via the JS hack).
If you're on any course-aware page,
- link goes to that course's Studio page.
If you're not on a course-aware page,
- link goes to main Studio homepage.

FWIW: We currently do not display the toolbar at all on course-unware pages.

stvstnfrd

Kind of a hack, but maybe not the Worst Thing Ever ™️

I may be totally missing some context (literally, the templating kind) on what data is available, but this seemed like the easiest way to fetch the current vertical ID (used for the Studio link/redirect on unit-level pages). Vertical-level context appears to be collected/passed further down the call stack, in a separate block.render call.

This is explained in further detail as code comments

stvstnfrd · 2020-05-19T02:38:07Z

openedx/core/lib/xblock_utils/__init__.py

Note for later: the // prefix..

Note: No algorithm, no helper method, so we weren't off by creating the URL the same way in the MFE :)

We found some similar limitations in studio for building LMS URLs... there wasn't really a good way to 'reverse' those routes, so to speak. Ended up looking much like this.

We found, also, that studio didn't have a fully reliable LMS base URL, in that if there were multi-tenancy or enterprise domains, it wasn't necessarily right. More details here: https://openedx.atlassian.net/browse/TNL-7198

stvstnfrd · 2020-05-19T02:39:52Z

lms/templates/edit_unit_link.html

stvstnfrd · 2020-05-19T04:08:11Z

lms/templates/preview_menu.html

TODO: Remove after local testing.

stvstnfrd · 2020-05-19T19:14:11Z

lms/djangoapps/courseware/tests/test_module_render.py

These tests seem fairly module-level specific, so probably not worth porting over as-is.

How much, if any, do we care to add testing here for the legacy view/component?
Spoiler: I've not added any at this point :)

stvstnfrd · 2020-05-19T19:16:14Z

lms/templates/preview_menu.html

Anchor tag hrefs work fine without an explicit protocol (eg: google.com or //google.com), but Javascript complains if you try to set window.location.href without an explicit protocol.

In devstack, stage, and prod, there is no protocol on CMS_BASE, but there is on ANALYTICS_DASHBOARD_URL.

stvstnfrd · 2020-05-19T20:48:54Z

Note: This doesn't look great on a smaller screen width; the buttons stack oddly...

marcotuts · 2020-05-19T20:56:35Z

If you're not on a course-aware page,
link goes to main Insights dashboard.
Thinking through this one... We may want View in Insights to only show on course-aware pages, or maybe I should ask what course-aware means. That is, View in Studio / View in Insights shouldn't show up on the masquerade bar for the progress page. but it should show up on Course Home + Courseware pages.

So perhaps there are three categories?

Non-Course Aware

dont' show view in studio or insights

Course Home

studio course outline page.
insights course home page

Course Content View

studio unit page
for now insights course home page, can add context sensitive mapping of unit pages to insights pages some other time / in a future ticket.

marcotuts · 2020-05-19T20:57:53Z

@stvstnfrd - For the button sizing we might need to make the width of the buttons wider than whatever percentage they have and make the masquerade area less of the width. Additionally on smaller screens the masquerade dropdown could be 100% and the buttons could be 100% so that they're stacked?

stvstnfrd · 2020-05-19T21:03:10Z

@marcotuts Yeah, how is described is what it is in practice now, since we don't show toolbar at all on non-course pages.

re 3, for the Insights link, I didn't know where else to link directly (a sensible unit-level insights page); if there's somewhere better, that's an easy enough change 🤷

marcotuts · 2020-05-20T13:55:13Z

For 3 - this is tricky because viewing a given unit page we might do different things. If viewing a page with a video we might wnat ot link to that video engagement view, if viewing a problem page then the problem performance page in Insights, but it becomes a mess when pages have both videos and problems, and other edge cases. I think for now ok to skip that level of context- sensitivity. Already this is a huge lift in the visibility of insights so we shall see how usage shifts and decide whether we should do anything else later. thanks!

stvstnfrd · 2020-05-29T02:07:58Z

lms/templates/preview_menu.html

@davidjoy Do you have any tricks on how to get these buttons to collapse better on smaller widths?
I'm happy to look for other examples in platform, just seeing if you have anything on hand ;)

If you add display: flex to this div, it'll cause the buttons to at least stay side-by-side. At very small widths (less than 460w on my screen) the insights button is partially off screen, but at least it doesn't take up more vertical space. Hm, I also don't have the MFE link enabled, but even if it was, I think they'd all stay on a line.

Beyond that, I think you have two options that I can think of:

You could vertically align the stuff in the toolbar - probably a non-starter because it'd take up so much space.

You could change them to an icon-based buttons instead of text, or a smaller font size, at narrower resolutions so that they take up less room. Or maybe a dropdown. You'd probably have to do this via media queries in CSS. Not that bad, but you'd have to play around a bit to get it right.

I think the latter is probably your best bet for making it "right" at small resolutions. Whether staff/instructors are really going to go look at studio/insights on a mobile phone, though... not clear to me it's worth it, though it galls me to leave it not-quite-fitting. :)

davidjoy · 2020-05-29T15:42:12Z

lms/templates/preview_menu.html

Could add a right margin instead of this  , probably.

davidjoy · 2020-05-29T15:46:47Z

openedx/core/lib/xblock_utils/__init__.py

I'm not 100% on the details here, but where is the equivalent of this check in the new code? Seems like in the old code we don't show the link unless the course_edit_method is "Studio", but I don't see a similar condition in the new stuff.

Hrmmm, good point...

We're also not performing this check in the MFE; we always link to Studio.

@davidjoy With this implemented as-is, I don't think we can perform this check; we don't have access to the block, so I think we'd need another approach.

@marcotuts (cc: @ormsbee) Do you have background on this feature, its use/exposure/etc?

Depending on that, we should determine:

If we care about this check, do we need to port this check to the MFE too? (now or backlog)

If we don't, do we care enough to preserve backward compatibility here? (now or backlog)

Or just ignore it and always link to the block in Studio?

davidjoy

Calling this a "Comment" review because it sounds like there are a few small changes to make and I had a few minor questions. In principle it's an Approve, though

stvstnfrd

Sooo, I tried quite a few things, but here's the TL;DR:
This scales down decently at smaller widths now.
It still stacks buttons as needed, but this is cleaned up and looks nicer. See:

It's "normal" on a single line down to about ~1,000px.

Drops down to a second line from ~600px to ~1,000px

And then puts each button on its own line below ~600px, and still looks decent down to ~400px, with room to spare.

I also confirmed that the "View as a specific user" still works at the smaller widths (it uses a bit more real estate). It's tight, but it's a lot of content 🤷

Note: px values here are rounded for ease of reference; no explicit media queries are used here; that's just roughly where the breaks occur.

stvstnfrd · 2020-06-02T15:29:35Z

lms/static/sass/course/layout/_courseware_preview.scss

Now it doesn't look as bad when they stack.

stvstnfrd · 2020-06-02T15:41:29Z

lms/static/sass/course/layout/_courseware_preview.scss

This handles the stacking display, as well as removing the need for the  .

stvstnfrd · 2020-06-02T15:49:15Z

openedx/core/lib/xblock_utils/__init__.py

Hrmmm, good point...

We're also not performing this check in the MFE; we always link to Studio.

@davidjoy With this implemented as-is, I don't think we can perform this check; we don't have access to the block, so I think we'd need another approach.

@marcotuts (cc: @ormsbee) Do you have background on this feature, its use/exposure/etc?

Depending on that, we should determine:

If we care about this check, do we need to port this check to the MFE too? (now or backlog)

If we don't, do we care enough to preserve backward compatibility here? (now or backlog)

Or just ignore it and always link to the block in Studio?

davidjoy · 2020-06-02T18:49:17Z

lms/templates/preview_menu.html

Could I suggest changing the align-items here to start instead of center? I think having the dropdown aligned to the top when the window width is shrunken down will look a bit cleaner.

Otherwise, I screwed around with this for a while just now and came up with much the same layout.

Hrmm, it might look a little better when shrunken down, but I think it may look worse when wide, since it's all on a single line and no longer centered vertically on the one side.

center, shrunk

start, shrunk

start, wide

center, wide

Oh, darn. Yup, looks like junk with start. Ah well.

davidjoy

Had a suggestion to change align-items to start, but otherwise think we're good to go.

in a feature backport from the new Courseware/Learning MFE.

in favor of display in the Staff Instructor Toolbar.

edx-status-bot · 2020-06-02T23:44:25Z

Your PR has finished running tests. There were no failures.

edx-pipeline-bot · 2020-06-03T19:25:57Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2020-06-03T19:37:28Z

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

edx-pipeline-bot · 2020-06-04T02:10:17Z

EdX Release Notice: This PR has been deployed to the production environment.

…utton openedx#24020 introduced a workaround for viewing the current vertical in Studio However, replacing `window.location.href` on `click` did not support opening it in a new tab.

…utton openedx#24020 introduced a workaround for viewing the current vertical in Studio However, replacing `window.location.href` on `click` did not support opening it in a new tab. (cherry picked from commit 01ada0b)

stvstnfrd requested a review from a team May 19, 2020 00:01

stvstnfrd commented May 19, 2020

View reviewed changes

stvstnfrd changed the title ~~WIP: Add Studio/Insights links to Staff Instructor Toolbar~~ Add Studio/Insights links to Staff Instructor Toolbar May 19, 2020

stvstnfrd force-pushed the add/staff-toolbar-links branch from 4aba540 to 0126aa6 Compare May 19, 2020 20:26

stvstnfrd requested review from davestgermain, davidjoy and ormsbee May 19, 2020 20:42

stvstnfrd commented May 29, 2020

View reviewed changes

davidjoy reviewed May 29, 2020

View reviewed changes

lms/templates/preview_menu.html Outdated

Copy link

Contributor

davidjoy May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a right margin instead of this  , probably.

davidjoy reviewed May 29, 2020

View reviewed changes

stvstnfrd commented Jun 2, 2020

View reviewed changes

davidjoy reviewed Jun 2, 2020

View reviewed changes

davidjoy approved these changes Jun 2, 2020

View reviewed changes

stvstnfrd added 2 commits June 2, 2020 16:00

Add Studio and Insights buttons to Instructor Toolbar

43ac90e

in a feature backport from the new Courseware/Learning MFE.

Remove 'View In Studio' link from vertical

4490e4c

in favor of display in the Staff Instructor Toolbar.

stvstnfrd force-pushed the add/staff-toolbar-links branch from 1461ff8 to 4490e4c Compare June 2, 2020 23:01

stvstnfrd merged commit 9dd97a5 into openedx:master Jun 3, 2020

stvstnfrd deleted the add/staff-toolbar-links branch June 3, 2020 16:39

Agrendalath mentioned this pull request Jun 1, 2021

fix: handle opening verticals in new tabs with the "View In Studio" button [BB-4211] #27799

Merged

2 tasks

Conversation

stvstnfrd commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Both Buttons

Insights Button

Studio Button

Uh oh!

stvstnfrd left a comment

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!

stvstnfrd commented May 19, 2020

Uh oh!

marcotuts commented May 19, 2020

Uh oh!

marcotuts commented May 19, 2020

Uh oh!

stvstnfrd commented May 19, 2020

Uh oh!

marcotuts commented May 20, 2020

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!

davidjoy left a comment

Choose a reason for hiding this comment

Uh oh!

stvstnfrd left a comment

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

center, shrunk

start, shrunk

start, wide

center, wide

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidjoy left a comment

Choose a reason for hiding this comment

Uh oh!

edx-status-bot commented Jun 2, 2020

Uh oh!

edx-pipeline-bot commented Jun 3, 2020

Uh oh!

edx-pipeline-bot commented Jun 3, 2020

Uh oh!

edx-pipeline-bot commented Jun 4, 2020

Uh oh!

Reviewers

stvstnfrd commented May 19, 2020 •

edited

Loading

`center`, shrunk

`start`, shrunk

`start`, wide

`center`, wide