Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to summary when toggling from standards view of csf to non-csf #32437

Merged
merged 9 commits into from Dec 20, 2019

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Dec 17, 2019

spec
LP-957 Part 2

The Standards view of the Progress tab of the Teacher Dashboard is only relevant to CSF courses. In #32430 I handle hiding/showing the standards button in the toggle based on whether the script selected is a CSF course. In this PR, I handle the case where a teacher switches from a CSF course to a non-CSF course while in the Standards view. The Standards view is irrelevant for non-CSF courses so we default the teacher to the Summary view in this case.
toggle-standards-content

// currentView is Standards. If so re-set currentView to Summary since
// Standards doesn't apply.
const isCSFBefore =
prevProps.scriptData && !prevProps.scriptData.excludeCsfColumnInLegend;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename excludeCsfColumnInLegend since we are using it for more than just that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pull-the-string-unravel-the-sweater-situation because I think we can actually eliminate excludeCsfColumnInLegend as a script property in favor of curriculum_umbrella == CSF. That seems worth doing and timely but also big enough to be its own work item, so I'll tackle that then circle back to make complementary changes here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for tackling that @Erin007 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#32470 is out for review

Copy link
Contributor

@dmcavoy dmcavoy left a comment

Choose a reason for hiding this comment

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

Looks good! Just one question

Copy link

@clareconstantine clareconstantine left a comment

Choose a reason for hiding this comment

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

I agree with Dani's suggestion to rename that variable, but otherwise looks good to me!

if (
prevProps.currentView === ViewType.STANDARDS &&
isCSFBefore &&
!isCSFAfter
Copy link
Member

Choose a reason for hiding this comment

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

great catch! I like what you did here, but I think you can make it a bit simpler and not look at the previous state:

const isCSF =
      this.props.scriptData && !this.props.scriptData.excludeCsfColumnInLegend;
    if (
      this.props.currentView === ViewType.STANDARDS &&
      !isCSF

@Erin007 Erin007 changed the base branch from standards-csf-only to staging December 19, 2019 04:39
@Erin007 Erin007 merged commit 488427b into staging Dec 20, 2019
@Erin007 Erin007 deleted the csf-to-non-csf branch December 20, 2019 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants