Tina/new student show #187

chtinahow · 2018-02-16T20:26:58Z

Resolves #182
This does the graphql query for all of the student side for the status.
This includes the new prototype of what Rob made shown here:

billdybas

You also need to lint your files, and the "Back" buttons on each submission form broke because of the URL changes.

billdybas · 2018-02-19T14:51:29Z

frontend/src/Student/components/ShowCard.js

+        <h5>{props.entries ? props.entries.length : 0}/{props.entryCap} submissions</h5>
+      </div>
+      <div>
+        Submission Ends: <Moment format='YYYY/MM/DD'>{props.entryEnd}</Moment>


I'd make this in a format that's more readable / glanceable (eg. March 5, 2018).

I'd also word it as "Accepting Submissions Until"

Also, we seem to have different wording between the backend and frontend: should we call them "Submissions" or "Entries"?

I think they should be submissions as that's what Nanette usually uses, but @robmcl4 @Kayladavis opinions?

Also, I don't think react-moment would be valid anymore if we decided to go with the glance able format

I agree with "submissions". At some point we just chose entries as types because it was slightly shorter of a word.

Also, I agree with Bill for the date format and wording.

billdybas · 2018-02-19T14:57:08Z

frontend/src/Student/components/ShowCard.js

+  judgingStart: PropTypes.string.isRequired,
+  judgingEnd: PropTypes.string.isRequired
+}
+


I would change entries to be required, and you should give it a defaultProp:

defaultProps = { entries: [] }

billdybas · 2018-02-19T15:00:15Z

frontend/src/Student/components/ShowCard.js

+  </Card>
+)
+
+ShowCard.propTypes = {


Instead of having all these props passed in shallowly, I would encapsulate them in a show prop:

ShowCard.propTypes = { show: PropTypes.shape({ id: PropTypes.string, ... judgingEnd: PropTypes.string }).isRequired }

See comment about entries below

billdybas · 2018-02-19T15:02:27Z

frontend/src/Student/components/ShowCard.js

+import FaPlusCircle from 'babel-loader!react-icons/fa/plus-circle'
+import { Button, ButtonGroup, Row, Col } from 'reactstrap'
+
+const Card = styled.div`


Visually, this feels too narrow. min-height: 250px seems to work well. You'll have to recenter the "New Submission" button.

billdybas · 2018-02-19T15:06:51Z

frontend/src/Student/components/ShowCard.js

+      </Col>
+      <Col className="text-right">
+      <div>
+        <h5>{props.entries ? props.entries.length : 0}/{props.entryCap} submissions</h5>


With the defaultProps below, props.show.entries.length will be zero when there are no submissions, so you could simplify this to just {props.show.entries.length}/{props.show.entryCap}

Also, see comment below about "Submissions" vs. "Entries"

Oh true, good catch

So doing so without the ternery causes a null if the entries are completely empty shown here in this error:

billdybas · 2018-02-19T15:17:55Z

frontend/src/Student/components/ShowCard.js

+  <Card>
+    <Row>
+      <Col>
+        <h2>{props.name}</h2>


I would align this text so the baseline is the same as "Submission Ends"

You want it centered with Accepting Submissions Until correct?

billdybas · 2018-02-19T15:19:25Z

frontend/src/Student/components/OtherMediaSubmissionForm.js

@@ -30,10 +30,6 @@ class OtherSubmissionForm extends Component {
    user: PropTypes.shape({
      username: PropTypes.string
    }).isRequired,
-    forShow: PropTypes.shape({


If you're removing this in each of these files, you need to add in the propTypes for data

billdybas · 2018-02-19T15:22:22Z

frontend/src/Student/queries/showName.graphql

@@ -0,0 +1,6 @@
+query Show($id: ID!) {


It feels weird to me making a query just for the name, but I guess it works: if we're coming to this page from the chooser, apollo probably has the data cached. If we're loading this page new, we'll need to fetch the name.

Still getting used to GraphQL ¯\_(ツ)_/¯

billdybas · 2018-02-19T15:27:10Z

frontend/src/Student/components/Shows.js

+)
+
+Shows.propTypes = {
+  shows: PropTypes.array,


I'd make this required and add a defaultProp of []

If there are no so associated with the student, would they be able to still log in?

billdybas · 2018-02-19T15:29:53Z

frontend/src/Student/pages/Submit.js

-      <Route exact path='/submit/photo' render={() => <PhotoSubmissionForm forShow={props.forShow} />} />
-      <Route exact path='/submit/video' render={() => <VideoSubmissionForm forShow={props.forShow} />} />
-      <Route exact path='/submit/other' render={() => <OtherMediaSubmissionForm forShow={props.forShow} />} />
+      <Route exact path='/submit/:id' component={SubmissionFormChooser} />


Not a huge fan of this URL format here.

Thoughts about doing:

/submit?to=<public_id>
/submit/photo?to=<public_id>
/submit/video?to=<public_id>
/submit/other?to=<public_id>

or

/submit?to=<public_id>
/submit?to=<public_id>&type=photo
/submit?to=<public_id>&type=video
/submit?to=<public_id>&type=other

Without the to param or an invalid/non-authorized public_id, you'd be redirected to your dashboard.

See #169 for my thoughts on using a public_id. We could just use the show's id for now.

Probably going with /submit?to=<public_id>
/submit/photo?to=<public_id>
/submit/video?to=<public_id>
/submit/other?to=<public_id>

kaylaerdmann

Nit and some questions. Bill covers a lot in his review.

kaylaerdmann · 2018-02-20T04:41:31Z

frontend/src/Student/containers/PhotoSubmissionForm.js

  previewImage: state.student.ui.submission.previewFile || {},
-  user: state.shared.auth.user
+  user: state.shared.auth.user,


Nit: extra comma

kaylaerdmann · 2018-02-20T04:47:41Z

frontend/src/Student/queries/shows.graphql

+    judgingEnd
+    entryCap
+    entries {
+      id


Will we need more than just the id?

I think so? Would the invited status be associated with the entries?

'baesline' is not a word

billdybas · 2018-02-27T12:11:36Z

frontend/src/Student/pages/Submit.js


 import SubmissionFormChooser from '../components/SubmissionFormChooser'
 import PhotoSubmissionForm from '../containers/PhotoSubmissionForm'
 import VideoSubmissionForm from '../containers/VideoSubmissionForm'
 import OtherMediaSubmissionForm from '../containers/OtherMediaSubmissionForm'

-const Submit = props => (
+const addIdPropFromQueryParams = () => {
+  return window.location.search.split('=')[1]


This is not the most robust way to do this, as any additional query parameters will cause issues.

eg. /submit?foo=bar&to=1 causes the buttons to link to /submit/<type>?to=bar

Additionally, no check is done to make sure a show w/ the id exists, so you can view the chooser for any id and when you attempt to view the submission forms for that non-existent show, the app crashes.

Ideally, we would parse the query string specifically for the param to and check if the id in the to query param is a valid show which this student can submit to, otherwise, render a Not Found page.

For MVP purposes, this'll work, but I'm making a story for having more robust query string parsing.

chtinahow added 2 commits February 16, 2018 14:17

Add the student show query and the intial empty card

2505317

id's to route for refreshable urls

6476b4d

chtinahow requested review from billdybas and kaylaerdmann February 16, 2018 20:27

billdybas requested changes Feb 19, 2018

View reviewed changes

kaylaerdmann reviewed Feb 20, 2018

View reviewed changes

chtinahow added 2 commits February 20, 2018 20:57

Change links to use query params, change submission nomenclature

6410dfb

Fix back button issues and some changes requested as per B$ request

a6f8d9a

robmcl4 mentioned this pull request Feb 22, 2018

[WIP] show student's entries on their dashboard #193

Closed

chtinahow and others added 6 commits February 23, 2018 09:18

Resolve datatype issue as some of the baseline issues, poorly

d17458e

Center New Submission button

16c7084

Fix linter issues except importing webpack items

bfcd972

Resolve other linting issues in modified files

2768b0d

Merge branch 'master' into tina/newStudentShow

b962c4d

Encapsulate 'Show' Prop & Actually Align Text Baseline

bb54002

'baesline' is not a word

billdybas reviewed Feb 27, 2018

View reviewed changes

billdybas approved these changes Feb 27, 2018

View reviewed changes

billdybas merged commit 401fcbd into master Feb 27, 2018

billdybas deleted the tina/newStudentShow branch February 27, 2018 12:13

billdybas mentioned this pull request Feb 27, 2018

As a developer, I want more robust query string parsing & not found checks #195

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tina/new student show #187

Tina/new student show #187

chtinahow commented Feb 16, 2018

billdybas left a comment

billdybas Feb 19, 2018

chtinahow Feb 19, 2018

chtinahow Feb 19, 2018

kaylaerdmann Feb 20, 2018

billdybas Feb 19, 2018

billdybas Feb 19, 2018

billdybas Feb 19, 2018

billdybas Feb 19, 2018

chtinahow Feb 19, 2018

chtinahow Feb 19, 2018

billdybas Feb 19, 2018

chtinahow Feb 19, 2018

billdybas Feb 19, 2018

billdybas Feb 19, 2018

billdybas Feb 19, 2018

chtinahow Feb 20, 2018

billdybas Feb 19, 2018

chtinahow Feb 20, 2018

kaylaerdmann left a comment

kaylaerdmann Feb 20, 2018

kaylaerdmann Feb 20, 2018

chtinahow Feb 20, 2018

billdybas Feb 27, 2018

Tina/new student show #187

Tina/new student show #187

Conversation

chtinahow commented Feb 16, 2018

billdybas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaylaerdmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment