-
Notifications
You must be signed in to change notification settings - Fork 47
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
New: advanced tab with Number tools extension code integrated #613
Conversation
…om:wpovernight/woocommerce-pdf-invoices-packing-slips into 606-improve-debug-tab-include-number-tools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it works as intended.
the one thing i noticed was that in testing the danger zone options, i wonder why this section is placed under the Tools tab and not under the Number tab when activated? i had automatically checked the Number tab after i activated the danger zone setting since that setting is exclusive to the number tools
@dwalkerpriv because all the tools are in one place, and I thought it would make sense adding those there as well. Don't forget that in the extension they were also separated with tabs, and the renumber/delete was called tools as well. If you need to take an action, I think makes more sense inside the tools. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran all the tools operations with all the documents again, just in case, and everything worked perfectly, as expected.
However, I'm leaving some comments below...
'settings' => __( 'Settings', 'woocommerce-pdf-invoices-packing-slips' ), | ||
'status' => __( 'Status', 'woocommerce-pdf-invoices-packing-slips' ), | ||
'tools' => __( 'Tools', 'woocommerce-pdf-invoices-packing-slips' ), | ||
'numbers' => __( 'Numbers', 'woocommerce-pdf-invoices-packing-slips' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could be more explicit with the tab name. What do you think about "Document numbers" or "Number logs"? I also like "Document numbers logs", but it can be too much longer 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, but because the other tabs have short names it felt strange to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's OK, please ignore my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the others could also give their input, I'm open to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "Numbers" is good. As Alex mentioned, it feels strange around other short names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer "Document Numbers" as its more descriptive. However if "Numbers" is preferable, we can adjust the description to something like this: "This tab has a record of all of the document numbers generated since the last reset (which happens when you set the next Invoice number value in the settings)."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Terminator-Barbapapa check now using the last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
closes #606
This PR introduces changes to the old Debug tab.