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

Added the logic in queue.js to show the queue name and location #81

Merged
merged 8 commits into from
Mar 30, 2018

Conversation

sgorse
Copy link
Collaborator

@sgorse sgorse commented Mar 28, 2018

No description provided.

Copy link
Member

@nwalters512 nwalters512 left a comment

Choose a reason for hiding this comment

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

Thanks Shirdon! Take a look at my feedback, and also please fix the linter errors that Travis is reporting. Last step would be to get up-to-date with master and add a changelog entry.

pages/queue.js Outdated
@@ -90,6 +95,7 @@ Queue.propTypes = {
const mapStateToProps = (state, ownProps) => ({
isFetching: state.queues.isFetching,
hasQueue: !!state.queues.queues[ownProps.queueId],
queueInfo: state.queues.queues[ownProps.queueId],
Copy link
Member

Choose a reason for hiding this comment

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

Should be named queue for consistency with the rest of the application.

pages/queue.js Outdated
@@ -46,7 +47,8 @@ class Queue extends React.Component {
}

render() {
const { isFetching, hasQueue } = this.props
const { isFetching, hasQueue, queueInfo } = this.props
const location = this.props.queueInfo.location || 'Not Available'
Copy link
Member

Choose a reason for hiding this comment

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

queueInfo might not be defined while the page is still loading (try refreshing the queue page with your code as-is). Avoid accessing any properties until after the various isFetching checks.

pages/queue.js Outdated
@@ -19,6 +19,7 @@ import QuestionNotificationsToggle from '../components/QuestionNotificationsTogg

class Queue extends React.Component {
static getInitialProps({ isServer, store, query }) {
console.log(this)
Copy link
Member

Choose a reason for hiding this comment

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

Remove extraneous log.

pages/queue.js Outdated
@@ -56,6 +58,9 @@ class Queue extends React.Component {
return (
<Layout>
<Container fluid>
<h5>
Copy link
Member

Choose a reason for hiding this comment

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

Try splitting into two lines, maybe with a combination of <h3>/<h5 className="text-muted">, would look even better with the location icon that's used on the queue cards on the homepage!

@sgorse
Copy link
Collaborator Author

sgorse commented Mar 29, 2018

I am having an issue fixing one of the linter errors. The initial error is " 'queue' is missing in props validation". When I try an add "queue: Proptypes.object.isRequired," to Queue.propTypes, it tells me that the Prop type object is forbidden. How can I add queue to the proptypes if its type is forbidden?

@nwalters512
Copy link
Member

You should use PropTypes.shape instead, take a look at the course prop on the course page.

@sgorse
Copy link
Collaborator Author

sgorse commented Mar 29, 2018

Ok, thanks for pointing that out. I am still getting a warning on the client console for the queue.id. I am getting this warning along with other warnings that do not pertain to the queue. The one that does is "Warning: Failed prop type: Invalid prop queue.id of type number supplied to Queue, expected object". Am I not setting the queue.id propType properly here?

Queue.propTypes = {
  isFetching: PropTypes.bool.isRequired,
  hasQueue: PropTypes.bool.isRequired,
  fetchQueue: PropTypes.func.isRequired,
  queueId: PropTypes.number.isRequired,
  dispatch: PropTypes.func.isRequired,
  queue: PropTypes.objectOf(
    PropTypes.shape({
      id: PropTypes.number,
      name: PropTypes.string,
      location: PropTypes.location,
    })
  ),
}

@sgorse
Copy link
Collaborator Author

sgorse commented Mar 29, 2018

Warning: Failed prop type: The prop descText is marked as required in ConfirmLeaveQueueModal, but its value is undefined.

main.js:4542 Warning: Failed prop type: The prop confirmText is marked as required in ConfirmLeaveQueueModal, but its value is undefined.

These were the other two warnings, but I don't think queue.js should have an effect on them

@nwalters512
Copy link
Member

You don't need the objectOf part, that's only if you have an object whose values are all queues. Just shape is enough. The other bit is not related to you: #79

pages/queue.js Outdated
return (
<Layout>
<Container fluid>
<h3>{this.props.queue.name}</h3>
<h5 className="text-muted">
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a bottom margin here? Maybe mb-4.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

         <Col
            xs={{ size: 12 }}
            md={{ size: 4 }}
            lg={{ size: 3 }}
            className="mb-3"
          >
            <h3>{this.props.queue.name}</h3>
            <h5 className="text-muted">
              <FontAwesomeIcon icon={faMapMarker} fixedWidth className="mr-2" />
              {locationText}
            </h5>
          </Col>

Would this be how I add the bottom margin?

Copy link
Member

Choose a reason for hiding this comment

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

You can just add it directly to the h5

@nwalters512 nwalters512 merged commit fedcddf into master Mar 30, 2018
@nwalters512 nwalters512 deleted the queueNameAndLoc branch March 30, 2018 18:38
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.

2 participants