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

Create "Tax Detail" view in Orders Dashboard #2005

Merged
merged 116 commits into from
Mar 21, 2017

Conversation

joykare
Copy link
Contributor

@joykare joykare commented Mar 16, 2017

Resolve #1857
Related to #1849

Description

Require a view where it can display the taxes charged per line item. Redesign UI on Invoice section on the Orders Dashboard.

Testing

  1. Create an order
  2. Process the order
  3. On the Invoice section click on a line item

screen shot 2017-03-16 at 1 07 47 am

  1. Expect a drop down list with inventory details for individual items

screen shot 2017-03-16 at 1 27 27 am

  1. Approve the order and Capture Payment

  2. Expect a captured payment popup

screen shot 2017-03-16 at 1 11 11 am

7. Refund an amount. Expect details on refunded amount and adjusted payment detail.

mikemurray and others added 30 commits February 15, 2017 19:17
* Have base to be release20

* Nitpick change on index
* Convert social settings to React with new cards

* Save card open state

* Add new SettingsCard component

 - SettingsCard component wraps many components necessary for a
dashboard settings card.
- Added method for updating user preferences

* Added a custom React semi-auto Form component

To help standardize all settings cards a new `Form` component has been
added that can use `SimpleSchema` to generate a basic, single level
form with validation.

* Update social settings and form component

- Add success message on submit
- Remove unused code
- Fix glitch with auto form and error messages

* Added prop type for switchName
* Add the ability to switch PDP template

Adds a select box to the PDP Admin view which allows a user to switch
between different registered templates.

* Add a templateFor array for tagging templates for specific views

* Removed console log

* Update select with i18n
* Added switch component to list item

* Remove unused prop
* Remove broken check for bulk write support

* Check that batch is not empty before trying to execute it
* Add a spinner to capture payment button

* Add conditional display of spinner

* Add loading state onto capture payment button

* Add loading state for refunds

* Disable the buttons when spinner is displayed/ when loading

* Remove unnecessary import

* Add loading state while waiting for refunds
* remove unused email setting dashboard tab

* camelCase filenames

* pull

* remove unused templates

* remove unneeded limit function (griddle includes this)

* remove no longer needed functionality

* update email publication

* remove react-bootstrap

* i18n updates

* add new sizes to actionView via registry

* customize griddle columns for data display

* remove react-bootstrap

* switch TextField to Reaction version

* pass correct props to select field

* move i18n into package

* ALL CAPS

* remove FieldGroup component

* remove fieldGroup component

* remove fieldGroup component

* remove react-bootstrap
* Update product handle when title changes

Ensure the handle is always set to the slugified product title, custom
input from user, the product id.

* Remove log statement

* Redirect on handle change

* Fix product grid link to use published product handle, not revision

* Better handling for validation errors

- Catch collection error
- Force update component with validation error so blank fields like
product title, will get the original title put back to keep it from
being blank

* Update logic for product handle update
@joykare joykare force-pushed the joykare-taxdetail-view-1857 branch from e7c9c8a to 9e2f2c2 Compare March 17, 2017 09:51
@brent-hoover
Copy link
Collaborator

When I click on the "print" or "print invoice" link I get a 404

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

If we can just fix the print link it's all looks good to me

@brent-hoover brent-hoover dismissed their stale review March 17, 2017 12:57

Joy says the print is working for her so if someone else could test that. Otherwise it looks good

@brent-hoover brent-hoover changed the title [WIP] Create "Tax Detail" view in Orders Dashboard Create "Tax Detail" view in Orders Dashboard Mar 18, 2017
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

tested and everything looks good to me

constructor(props) {
super(props);

this.renderConditionalDisplay = this.renderConditionalDisplay.bind(this);
Copy link
Member

@mikemurray mikemurray Mar 20, 2017

Choose a reason for hiding this comment

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

Do these need to be bind-ed to the component instance? You should only need to do that in the case of event handlers for onClick and etc. Normal function calls in a class generally don't need to be explicitly bound.

And in those cases you can do the following as we have class properties enabled through babel.

myFunctionName = () => {

}

}
}

Invoice.propTypes = {
Copy link
Member

@mikemurray mikemurray Mar 20, 2017

Choose a reason for hiding this comment

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

This could be moved into the class as:

class Invoice extends Component {
    static propTypes = {}

    ...

Though the way you did it is fine too, I've just been moving the to new style when I can.

@@ -9,7 +9,7 @@ export default class DiscountForm extends Component {
this.state = {
discount: this.props.discount,
validationMessage: null,
validatedInput: false,
validatedInput: false || this.props.validatedInput,
Copy link
Member

Choose a reason for hiding this comment

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

should this be reversed as the false will always execute the second part of that statement.

this.props.validatedInput || false

@mikemurray
Copy link
Member

Just some syntax things for my review, otherwise everything looks really good 👍

@mikemurray
Copy link
Member

When you call this.renderConditionalDisplay(); in a React component/ ES6 class, the function you define on the class should be renderConditionalDisplay() {}.

When you attach an event handler to a button, like so <button onClick={this.handleClick} />, then your class function should look like onClick = () => {}. That way you bind a reference to this class to it.

For example

class MyComponent extends Component {
  handleClick = () => {
    // Handle button click
  }
  
  renderSomething() {
    return (
      <button onClick={this.handleClick}>{"Something Else"}</button>
    )
  }

  render() {
    return (
      <div>
        {this.renderSomething()}
      </div>
    )
  }
}

From the React documentation

Generally, if you refer to a method without () after it, such as onClick={this.handleClick}, you should bind that method.

Our Switch component is a good example of style I hope to go for as we work towards going all React. https://github.com/reactioncommerce/reaction/blob/master/imports/plugins/core/ui/client/components/switch/switch.js

@joykare
Copy link
Contributor Author

joykare commented Mar 21, 2017

@mikemurray Changed it. Hope I got it right this time.

@mikemurray
Copy link
Member

looks good

@brent-hoover brent-hoover removed the request for review from kieha March 21, 2017 19:44
@brent-hoover brent-hoover merged commit 261a351 into development Mar 21, 2017
@brent-hoover brent-hoover deleted the joykare-taxdetail-view-1857 branch March 21, 2017 21:50
@joykare joykare self-assigned this Mar 22, 2017
jshimko added a commit to evereveofficial/reaction that referenced this pull request Mar 27, 2017
* development:
  fix elasticsearch errors
  skip npm install outside of container on CI
  wait longer on CI for Docker test
  update CI config for Evereve
  add ReactionPublicCustomFolder
  update data_import
  update data_import
  update data_import
  add submodules to circle config
  add plugin submodules
  Don't reload when the main product is not in tbe modified products (reactioncommerce#2033)
  If default provider is not available, then add it. (reactioncommerce#2025)
  Dashboard panel keeps re-opening during checkout (reactioncommerce#2010)
  fixing issue 2008 getting 404 error on variant option (reactioncommerce#2021)
  Create "Tax Detail" view in Orders Dashboard (reactioncommerce#2005)
@aaronjudd aaronjudd mentioned this pull request Mar 28, 2017
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.

7 participants