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

Progress tab: Initial work #19881

Merged
merged 14 commits into from Jan 11, 2018
Merged

Progress tab: Initial work #19881

merged 14 commits into from Jan 11, 2018

Conversation

Bjvanminnen
Copy link
Contributor

@Bjvanminnen Bjvanminnen commented Jan 9, 2018

This is some initial work towards getting the progress tab using React

image

It is behind the flag sectionProgressRedesign and should result in no changes if you don't have the flag enabled.

@Bjvanminnen Bjvanminnen changed the title WIP: Progress tab Progress tab: Initial work Jan 11, 2018
import React, { Component, PropTypes } from 'react';
import _ from 'lodash';

// TODO: Can/should we share any logic with AssignmentSelector?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The answer might be that they're just different.

This is a little confusing in that they use slightly difference sources of data for their list of scripts. AssignmentSelector uses a set of assignments that have been slightly processed, to be able to differentiate scripts and courses. In part this involves giving each one an "assignment id". That said, both have the same set of fields used for grouping I believe.

* show progress for (selected via a dropdown), and querying the server for
* student progress. Child components then have the responsibility for displaying
* that progress.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to separate the data gathering component from the UI component for the most part. That said, this component does still include the dropdown.

@@ -4,90 +4,92 @@ theme: none
content-type: text/ng-template
---

%link(rel='stylesheet' type='text/css' href='/shared/css/teacher-announcement.css')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of the changes here are whitespace only

@Bjvanminnen
Copy link
Contributor Author

There's still more cleanup to be done here (evidenced in part by some remaining TODOs), but this felt like an okay stopping point to get some of this committed at least.

@caleybrock
Copy link
Contributor

👀

return;
}

// The below is not run if our experiment is not enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be more specific about the experiment here. There are a few going at a time in teacher dashboard.

scriptId={scriptId}
onChange={this.onChangeScript}
/>
{!levelDataInitialized && <div>Loading...</div>}
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: this is the spinner we normally use
<FontAwesome icon="spinner" className="fa-pulse fa-3x"/>

students: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
name: PropTypes.string.isRequired,
})).isRequired
Copy link
Contributor

Choose a reason for hiding this comment

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

it's possible this array is empty - will that still satisfy isRequired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so.

"icon": "fa-check-square-o",
"is_concept_level": false,
"title": 1,
"url": "http://localhost-studio.code.org:3000/s/csp1/lockable/1/puzzle/1/page/1",
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to do a relative or prod url here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it really matters. It's not really designed for clicking through in storybook. Mostly I just wanted the story so that we have at least some test coverage.

%link(rel='stylesheet' type='text/css' href='/shared/css/teacher-announcement.css')
#section-progress-react{'ng-init' => '$emit("section-progress-rendered");', 'ng-show' => 'react_progress'}
#section-progress-angular{'ng-hide' => 'react_progress'}
%link{:rel => 'stylesheet', :type => 'text/css', :href => '/shared/css/teacher-announcement.css'}
Copy link
Contributor

Choose a reason for hiding this comment

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

are links meant to be nested like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It appeared to work for me, but it might be safer to leave the stylesheet outside of the div.

@Bjvanminnen Bjvanminnen merged commit b70a718 into staging Jan 11, 2018
@Bjvanminnen Bjvanminnen deleted the progressTab branch January 11, 2018 22:11
@@ -0,0 +1,62 @@
import React, { Component, PropTypes } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this is a js file not jsx, on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

either is fine. I think I usually use .js

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

2 participants