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

Feat/et 2163 status notices #3210

Open
wants to merge 14 commits into
base: bucket/order-management-p2b
Choose a base branch
from

Conversation

stratease
Copy link
Contributor

@stratease stratease commented Aug 29, 2024

🎫 Ticket

ET-2163

🗒️ Description

Status notices.

🎥 Artifacts

image

✔️ Checklist

  • Ran npm run changelog to add changelog file(s). More info here
  • Code is covered by NEW wpunit or integration tests.
  • Code is covered by EXISTING wpunit or integration tests.
  • Are all the required tests passing?
  • Automated code review comments are addressed.
  • Have you added Artifacts?
  • Check the base branch for your PR.
  • Add your PR to the project board for the release.

@stratease stratease added the code review Status: requires a code review. label Aug 29, 2024
@stratease stratease self-assigned this Aug 29, 2024
Camwyn
Camwyn previously approved these changes Aug 29, 2024
Copy link
Member

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

Love it overall! Left a few suggeestions

dpanta94
dpanta94 previously approved these changes Sep 3, 2024
…alendar/event-tickets into feat/ET-2163-status-notices
…events-calendar/event-tickets into feat/ET-2163-status-notices"

This reverts commit 2b8d6b0, reversing
changes made to d5a19e7.
…com:the-events-calendar/event-tickets into feat/ET-2163-status-notices""

This reverts commit 2badbfa.
dpanta94
dpanta94 previously approved these changes Sep 19, 2024
Copy link
Member

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

Just a phpcs and good to go

/home/runner/work/event-tickets/event-tickets/src/Tickets/Commerce/Admin/Singular_Order_Page.php:73:1:Generic.WhiteSpace.DisallowSpaceIndent.SpacesUsed<br>Tabs must be used to indent lines; spaces are not allowed

Copy link
Member

@dpanta94 dpanta94 left a comment

Choose a reason for hiding this comment

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

And some stylelint issues :D

*
* @package TEC\Tickets\Commerce\Admin
*/
class Singular_Order_Notices {
Copy link
Member

Choose a reason for hiding this comment

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

I would love to chat tomorrow about this particular implementation before we merge, I have some thoughts on potentially using some other pattern that doesn't involve as many constants.

Comment on lines 22 to 28
/**
* @since TBD
*
* @var string Notice key.
*/
const ORDER_STATUS_SUCCESSFULLY_UPDATED = 'updated';
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

These all need a space between them.

Suggested change
/**
* @since TBD
*
* @var string Notice key.
*/
const ORDER_STATUS_SUCCESSFULLY_UPDATED = 'updated';
/**
/**
* @since TBD
*
* @var string Notice key.
*/
const ORDER_STATUS_SUCCESSFULLY_UPDATED = 'updated';
/**

/**
* Success messages.
*/
/* translators: %1$s: order number. %2$s: status the order was changed from. %3$s: status the order was changed to. */
Copy link
Contributor

@JPry JPry Sep 23, 2024

Choose a reason for hiding this comment

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

For these comments, you don't need to include the extra characters in the placeholders.

Suggested change
/* translators: %1$s: order number. %2$s: status the order was changed from. %3$s: status the order was changed to. */
/* translators: 1: order number 2: status the order was changed from 3: status the order was changed to. */

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code review Status: requires a code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants