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

Staff router #33

Open
wants to merge 12 commits into
base: staff
Choose a base branch
from
Open

Staff router #33

wants to merge 12 commits into from

Conversation

adrian678
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@Gabetn Gabetn left a comment

Choose a reason for hiding this comment

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

General

What is the exact functionality meant to be accomplished by this PR & what is it expecting in terms of future development. Unsure on what is intentional vs unintentionally missing.

As is i cannot merge this into staff. It will have to be refactored. As for the merge plan I shall merge staff into dev without this and should refactoring be done in time I will included it, however due to the deadline for release I shall merge dev into master without this if we cannot fix it in time. Please comment if you feel otherwise

See review comments for further details.

Styling

Seems to be entirely broken. Is this just local to my computer or were you experiencing the same @adrian678
image

Code Formatting

  1. Remove commented out console logs & other lines of unused code
  2. Renaming of functions, (optional) see specific comments in review

@@ -95,6 +96,25 @@ public void run(String... strings) {
menuItemRepository.save(menuItem);
}
}

// FILL ORDER TABLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removal of testing functionality (order's should not be pre-populated)
marking as a reminder

render() {
return (
<div className={"page staff-page"}>
<StaffLanding orders={this.props.orders}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not have been deleted.
Perhaps it was temporary, but staff landing page is the entry point to the staff view, and should remain.
StaffDiningSessionPage should be redirected to upon click of the All Orders button in the StaffLanding page.

@@ -3,6 +3,7 @@ import React from "react";
import {FontAwesomeIcon} from "@fortawesome/react-fontawesome";

/** ----- COMPONENT IMPORTS -----**/
import {CustomerDiningSessionSelect, StaffDiningSessionPage, StaffDiningSession} from "./subcomponents/DiningSession";
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of importing CustomerDiningSessionSelect as well as StaffDiningSession here?

)
}

handleClose(){
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should redirect to the staff landing page
However there doesn't seem to be an exit button on this page to trigger handleClose()

diningSession={session}
diningSessionAttributes={this.props.diningSessionAttributes}
history = {this.props.history}
onCreate={this.onCreate}
Copy link
Collaborator

Choose a reason for hiding this comment

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

onCreate does not need to be passed.
onUpdate/onDelete are not implemented throughout


componentWillUnmount(){
console.log("UNMOUNTING UNMOUNTING UNMOUNTING UNMOUNTING UNMOUNTING")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove.
Is this function necessary?

(
{
pathname: ('/staff/'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

/staff

{
pathname: ('/staff/'),
props: this.props
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems, unnecessary. What was the rationale behind it?

orderAttributes={this.props.orderAttributes}
onUpdate={this.props.onUpdate}
//TODO does Staff have authority to delete order?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not included in the stories we provided. As such it is not functionality we should implement

quantity: this.props.order.quantity,
menuItem: {}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Confused with the state initializations. What was the reasoning behind price : 4.99 in line 174

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.

3 participants