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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Session navigation scanner toast api examples for pos ui ext docs #1973

Merged
merged 2 commits into from
May 22, 2024
Merged

Session navigation scanner toast api examples for pos ui ext docs #1973

merged 2 commits into from
May 22, 2024

Conversation

vctrchu
Copy link
Contributor

@vctrchu vctrchu commented May 16, 2024 •
edited

Resolves https://github.com/Shopify/pos-next-react-native/issues/36279
Resolves https://github.com/Shopify/pos-next-react-native/issues/36288

Background

This PR adds the examples for navigation, session, scanner, toast apis to the docs.

Some important changes you can look below at the comments to find more information but

  • I added a extension target helper
  • You can edit the required field for docs to help us clarify which targets are needed for usage
  • Updating the formatter file to allow us to have consistent styling for ts and tsx files

馃帺

Checklist

  • I have 馃帺'd these changes
  • I have updated relevant documentation

@vctrchu vctrchu self-assigned this May 16, 2024
@vctrchu vctrchu force-pushed the vic/session-scanner-toast branch from d9de9fa to 2218eb9 Compare May 16, 2024 15:47
@vctrchu vctrchu changed the title Vic/session scanner toast Session navigation scanner toast api examples for pos ui ext docs May 16, 2024
vctrchu
vctrchu commented May 16, 2024
View reviewed changes
packages/ui-extensions/docs/surfaces/point-of-sale/reference/apis/action-api.doc.ts Outdated
@@ -16,6 +20,11 @@ const data: ReferenceEntityTemplateSchema = {
],
category: 'APIs',
related: [],
requires: `the ${returnExtensionTarget(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows us to specify which targets are required to be used for an API 馃槑
Screenshot 2024-05-16 at 2 13 43鈥疨M

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@merkoyep @NathanJolly @js-goupil good to know!

@vctrchu vctrchu requested a review from js-goupil May 16, 2024 21:20
@vctrchu vctrchu assigned NathanJolly and unassigned NathanJolly May 16, 2024
@vctrchu vctrchu requested a review from NathanJolly May 16, 2024 21:20
@vctrchu vctrchu force-pushed the vic/session-scanner-toast branch from 4d838c5 to ffc700e Compare May 16, 2024 21:22
@vctrchu vctrchu requested a review from merkoyep May 16, 2024 21:23
@vctrchu vctrchu marked this pull request as ready for review May 16, 2024 21:24
js-goupil
js-goupil reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@js-goupil js-goupil left a comment

Choose a reason for hiding this comment

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

Mostly looks ready! Just a couple comments. I'm going to wait on this to merge, since I think there's a lot of overlap in the Screen and Navigator doc examples and these ones. We'll be able to reuse the examples. Mine does add a sheet presentation example that I can add to the navigation API docs as well

...extensions/docs/surfaces/point-of-sale/reference/examples/navigation-api/navigation-tree.tsx
@@ -0,0 +1,8 @@
// You can navigate to any of these three screens since they all exist within the same Navigator.
return (
<Navigator>
Copy link
Contributor

Choose a reason for hiding this comment

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

This example seems incomplete compared to its TS counterpart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

...s/ui-extensions/docs/surfaces/point-of-sale/reference/examples/navigation-api/two-screen.tsx Outdated Show resolved Hide resolved
NathanJolly
NathanJolly reviewed May 21, 2024
View reviewed changes
...ions/docs/surfaces/point-of-sale/reference/examples/scanner-api/hardware-scanner-example.tsx Outdated
Comment on lines 18 to 24
const addProductToCart = (data: string | undefined) => {
if (data) {
api.cart.addLineItem(Number(data), 1);
}
};
addProductToCart(data);
}, [data]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't shopify assign productID's regardless of what the scanned data is? In other words, does this logic actually make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I need more clarification on this. I don't understand

Copy link
Contributor

Choose a reason for hiding this comment

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

The example says "whatever barcode is scanned, add the product with that productID to the cart"

I'm wondering if that example is misleading because a barcode doesn't necessarily match a productID. I think a more accurate example would be to just put the text on the screen somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also not async so I don't think there's a reason to do the whole "make a function call a function" thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (data) { api.cart.addLineItem(Number(data), 1); }

My understanding is more of "if theres a barcode data scanned then it will attempt to add that productID to the cart". If the productID doesn't exist than nothing happens to the cart. Maybe the description of the example can be written better.

I agree the async is excessive and not useful here so I'll update that as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

Right! My point is barcode !== productId, so trying to add the barcode as a productId will never match... Here's a pic of admin to illustrate
image

Copy link
Contributor

Choose a reason for hiding this comment

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

ProductId is that 760...104 number, and the barcode is a manually-entered user field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thank you for helping me understand! This logic is unnecessary I'll remove it

NathanJolly
NathanJolly approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@NathanJolly NathanJolly left a comment

Choose a reason for hiding this comment

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

Great!

@vctrchu vctrchu force-pushed the vic/session-scanner-toast branch from 3b28f75 to 120677e Compare May 22, 2024 19:30
@vctrchu vctrchu merged commit 4a0f71d into unstable May 22, 2024
4 checks passed
@vctrchu vctrchu deleted the vic/session-scanner-toast branch May 22, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NathanJolly NathanJolly NathanJolly approved these changes

@merkoyep merkoyep Awaiting requested review from merkoyep

@js-goupil js-goupil Awaiting requested review from js-goupil

Assignees

@vctrchu vctrchu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vctrchu @NathanJolly @js-goupil