Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions apps/src/code-studio/components/TeacherContentToggle.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ class TeacherContentToggle extends React.Component {
hiddenLessonsInitialized: PropTypes.bool.isRequired,
sectionsAreLoaded: PropTypes.bool.isRequired,
isHiddenLesson: PropTypes.bool.isRequired,
isLockedLesson: PropTypes.bool.isRequired
isLockedLesson: PropTypes.bool.isRequired,
isCodeReviewing: PropTypes.bool
};

componentDidMount() {
Expand Down Expand Up @@ -55,7 +56,8 @@ class TeacherContentToggle extends React.Component {
sectionsAreLoaded,
isLockedLesson,
isHiddenLesson,
isBlocklyOrDroplet
isBlocklyOrDroplet,
isCodeReviewing
} = this.props;

const frameStyle = {
Expand All @@ -71,9 +73,13 @@ class TeacherContentToggle extends React.Component {
let hasOverlayFrame = isLockedLesson || isHiddenLesson;

if (viewAs === ViewType.Participant) {
// When a teacher is code reviewing another teacher, we load a different header experience
// so that we don't expose progress between peers. Section data is not loaded in this teacher-as-student
// viewing another teacher experience
const sectionsAreLoading = !sectionsAreLoaded && !isCodeReviewing;
// Keep this hidden until we've made our async calls for hidden_lessons and
// locked lessons, so that we don't flash content before hiding it
if (!hiddenLessonsInitialized || !sectionsAreLoaded || hasOverlayFrame) {
if (!hiddenLessonsInitialized || sectionsAreLoading || hasOverlayFrame) {
contentStyle.visibility = 'hidden';
}
}
Expand Down Expand Up @@ -138,7 +144,7 @@ export const mapStateToProps = state => {
);
} else if (!state.verifiedInstructor.isVerified) {
// if not-authorized teacher
isLockedLesson = state.progress.lessons.some(
isLockedLesson = state.progress.lessons?.some(
Copy link
Contributor

Choose a reason for hiding this comment

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

So... we are only in this 'if' block when we have a teacher who is not viewingAs a participant, and is not verified? And the page was stuck because we weren't loading in progress, so there were no progress.lessons to look at? I think I vaguely understand what's going on but the url params are so messy that I'd love to someday rethink the entire teacher panel and how it gets data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you have it right. It is very confusing. After some further thought, I'm going to hide the teacher panel in this scenario altogether. It doesn't really make sense to render a teacher panel when viewing a teacher-peers work

Copy link
Contributor

Choose a reason for hiding this comment

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

Heck yeah to hiding the teacher panel- I endorse this message🙌🏼

lesson => lesson.id === currentLessonId && lesson.lockable
);
}
Expand All @@ -148,7 +154,8 @@ export const mapStateToProps = state => {
sectionsAreLoaded: state.teacherSections.sectionsAreLoaded,
hiddenLessonsInitialized: state.hiddenLesson.hiddenLessonsInitialized,
isHiddenLesson,
isLockedLesson
isLockedLesson,
isCodeReviewing: state.pageConstants?.isCodeReviewing
};
};

Expand Down
15 changes: 15 additions & 0 deletions apps/src/code-studio/components/header/HeaderMiddle.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const lessonProgressExtraWidth = 10;
class HeaderMiddle extends React.Component {
static propTypes = {
projectInfoOnly: PropTypes.bool,
scriptNameOnly: PropTypes.bool,
appLoadStarted: PropTypes.bool,
appLoaded: PropTypes.bool,
scriptNameData: PropTypes.object,
Expand Down Expand Up @@ -78,6 +79,7 @@ class HeaderMiddle extends React.Component {
static getWidths(
width,
projectInfoOnly,
scriptNameOnly,
projectInfoDesiredWidth,
scriptNameDesiredWidth,
lessonProgressDesiredWidth,
Expand All @@ -96,6 +98,18 @@ class HeaderMiddle extends React.Component {
};
}

if (scriptNameOnly) {
return {
projectInfo: 0,
scriptName: Math.floor(
Math.min(scriptNameDesiredWidth + scriptNameExtraWidth, width)
),
progress: 0,
popup: 0,
finish: 0
};
}

// We will take care of padding the lesson progress with an extra 5 pixels
// on each side.
const lessonProgressDesiredWidthAdjusted =
Expand Down Expand Up @@ -179,6 +193,7 @@ class HeaderMiddle extends React.Component {
const widths = HeaderMiddle.getWidths(
this.state.width,
this.props.projectInfoOnly,
this.props.scriptNameOnly,
this.state.projectInfoDesiredWidth,
this.state.scriptNameDesiredWidth,
this.state.lessonProgressDesiredWidth,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ export default connect(
);

let lessonNames = {};
state.progress.lessons.forEach(lesson => {
state.progress.lessons?.forEach(lesson => {
lessonNames[lesson.id] = lesson.name;
});

Expand Down
11 changes: 11 additions & 0 deletions apps/src/code-studio/header.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,17 @@ header.buildProjectInfoOnly = function() {
);
};

// When viewing the level page in code review mode, we want to show only the
// lesson information (which is displayed by the ScriptName component).
header.buildScriptNameOnly = function(scriptNameData) {
ReactDOM.render(
<Provider store={getStore()}>
<HeaderMiddle scriptNameData={scriptNameData} scriptNameOnly={true} />
</Provider>,
document.querySelector('.header_level')
);
};

// When the page is cached, this function is called to retrieve and set the
// sign-in button or user menu in the DOM.
header.buildUserMenu = function() {
Expand Down
25 changes: 18 additions & 7 deletions apps/src/sites/studio/pages/levels/_teacher_panel.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
setUserRoleInCourse,
CourseRoles
} from '@cdo/apps/templates/currentUserRedux';
import {queryParams} from '@cdo/apps/code-studio/utils';

$(document).ready(initPage);

Expand All @@ -34,16 +35,26 @@ function initPage() {
}

if (teacherPanelData.is_instructor) {
store.dispatch(setViewType(ViewType.Instructor));
store.dispatch(setViewType(queryParams('viewAs') || ViewType.Instructor));
store.dispatch(setUserRoleInCourse(CourseRoles.Instructor));
}

renderTeacherPanel(
store,
teacherPanelData.script_id,
teacherPanelData.script_name,
teacherPanelData.page_type
);
// If a teacher is peer-reviewing another teacher in a workshop, don't render
// the teacher panel, as it doesn't make sense in that context.
// We need to check for presence of appOptions since some pages such as /extras
// (lesson extras page) do not set appOptions.
const shouldRenderTeacherPanel = window.appOptions
? !window.appOptions.isCodeReviewing
: true;

if (shouldRenderTeacherPanel) {
renderTeacherPanel(
store,
teacherPanelData.script_id,
teacherPanelData.script_name,
teacherPanelData.page_type
);
}
}

function renderTeacherContentToggle(store) {
Expand Down
46 changes: 46 additions & 0 deletions apps/test/unit/code-studio/components/header/HeaderMiddleTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe('HeaderMiddle', () => {
const widths = HeaderMiddle.getWidths(
700, // width,
false, // projectInfoOnly,
false, // scriptNameOnly,
0, // projectInfoDesiredWidth,
200, // scriptNameDesiredWidth,
350, // lessonProgressDesiredWidth,
Expand All @@ -31,6 +32,7 @@ describe('HeaderMiddle', () => {
const widths = HeaderMiddle.getWidths(
350, // width,
false, // projectInfoOnly,
false, // scriptNameOnly,
0, // projectInfoDesiredWidth,
200, // scriptNameDesiredWidth,
350, // lessonProgressDesiredWidth,
Expand All @@ -52,6 +54,7 @@ describe('HeaderMiddle', () => {
const widths = HeaderMiddle.getWidths(
700, // width,
false, // projectInfoOnly,
false, // scriptNameOnly,
200, // projectInfoDesiredWidth,
200, // scriptNameDesiredWidth,
350, // lessonProgressDesiredWidth,
Expand All @@ -73,6 +76,7 @@ describe('HeaderMiddle', () => {
const widths = HeaderMiddle.getWidths(
350, // width,
false, // projectInfoOnly,
false, // scriptNameOnly,
200, // projectInfoDesiredWidth,
200, // scriptNameDesiredWidth,
350, // lessonProgressDesiredWidth,
Expand All @@ -94,6 +98,7 @@ describe('HeaderMiddle', () => {
const widths = HeaderMiddle.getWidths(
700, // width,
true, // projectInfoOnly,
false, // scriptNameOnly,
400, // projectInfoDesiredWidth,
0, // scriptNameDesiredWidth,
0, // lessonProgressDesiredWidth,
Expand All @@ -113,6 +118,7 @@ describe('HeaderMiddle', () => {
const widths = HeaderMiddle.getWidths(
350, // width,
true, // projectInfoOnly,
false, // scriptNameOnly,
400, // projectInfoDesiredWidth,
0, // scriptNameDesiredWidth,
0, // lessonProgressDesiredWidth,
Expand All @@ -127,4 +133,44 @@ describe('HeaderMiddle', () => {
assert.equal(widths.popup, 0);
assert.equal(widths.finish, 0);
});

it('widths for script name only when wide', () => {
const widths = HeaderMiddle.getWidths(
700, // width,
false, // projectInfoOnly,
true, // scriptNameOnly,
0, // projectInfoDesiredWidth,
400, // scriptNameDesiredWidth,
0, // lessonProgressDesiredWidth,
0, // numScriptLessons,
0, // finishDesiredWidth,
false // showFinish
);

assert.equal(widths.projectInfo, 0);
assert.equal(widths.scriptName, 410);
assert.equal(widths.progress, 0);
assert.equal(widths.popup, 0);
assert.equal(widths.finish, 0);
});

it('widths for script name only when narrow', () => {
const widths = HeaderMiddle.getWidths(
350, // width,
false, // projectInfoOnly,
true, // scriptNameOnly,
0, // projectInfoDesiredWidth,
400, // scriptNameDesiredWidth,
0, // lessonProgressDesiredWidth,
0, // numScriptLessons,
0, // finishDesiredWidth,
false // showFinish
);

assert.equal(widths.projectInfo, 0);
assert.equal(widths.scriptName, 350);
assert.equal(widths.progress, 0);
assert.equal(widths.popup, 0);
assert.equal(widths.finish, 0);
});
});
1 change: 1 addition & 0 deletions dashboard/app/controllers/script_levels_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ def load_user
@user = user_to_view

if can?(:view_as_user_for_code_review, @script_level, user_to_view, sublevel_to_view)
@is_code_reviewing = true
view_options(is_code_reviewing: true)
end
end
Expand Down
14 changes: 13 additions & 1 deletion dashboard/app/views/layouts/_header.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
header_contents_options[:loc_prefix] = "nav.header."
header_contents_options[:page_mode] = request.cookies['pm']

should_show_progress = script_level || @lesson_extras
should_show_progress = (script_level || @lesson_extras) && !@is_code_reviewing

sign_in_user = current_user || user_type && OpenStruct.new(
id: nil,
Expand Down Expand Up @@ -131,6 +131,12 @@
href: script_path(@script, section_id: section_id, user_id: user_id),
smallText: small_text
}
elsif @is_code_reviewing
script_name_data = {
name: script_level.lesson.localized_title,
href: '#', # When viewing peer code, link should do nothing
smallText: false
}
end

- if should_show_progress
Expand All @@ -150,6 +156,12 @@
)
//]]>

- elsif @is_code_reviewing
:javascript
dashboard.header.buildScriptNameOnly(
#{script_name_data.to_json}
)

- elsif level
:javascript
dashboard.header.buildProjectInfoOnly()
Expand Down