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

Add a simple page to list a few products from a GraphQL query. #27

Merged
merged 1 commit into from
May 19, 2021

Conversation

MuhammadFarag
Copy link
Contributor

WHY are these changes introduced?

Add a very simple page to list a few products from a GraphQL query, to demonstrate how to load a session / use it for requests.

WHAT is this pull request doing?

  • Proxy no longer passing headers
  • Downgrade React to 16.x as 17.x is not compatible with Polaris
  • Load and display products list

Checklist

  • I have added/updated tests for this change

Copy link
Collaborator

@paulomarg paulomarg left a comment

Choose a reason for hiding this comment

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

This is looking amazing, great job!

return (
<h1>Hello React</h1>
<AppProvider i18n={translations}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if you use <Provider> from app-bridge-react, you'll get the styling automatically so you don't have to directly include the CSS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added <Provider> but I still had to include the css, this is being done in the node app as well

resources/js/react/App.jsx Outdated Show resolved Hide resolved
resources/js/react/components/ProductsPage.jsx Outdated Show resolved Hide resolved
@@ -112,7 +112,7 @@

Route::post('/graphql', function (Request $request) {
$result = Utils::graphqlProxy($request->header(), $request->cookie(), $request->getContent());
return response($result->getDecodedBody())->withHeaders($result->getHeaders());
return response($result->getDecodedBody());
Copy link
Collaborator

Choose a reason for hiding this comment

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

If simply passing along all the headers doesn't work, we could come up with a list of the ones we want to set and go over the list here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good idea, I wasn't sure though what would be those ones. I will check the issue with the headers separately. Thought of putting it aside given that we do not using it right now.

Copy link
Collaborator

@paulomarg paulomarg May 19, 2021

Choose a reason for hiding this comment

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

Agreed! Though we probably also want to make sure to forward the response code we got back!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, let me have those two points in a different PR

@MuhammadFarag MuhammadFarag force-pushed the sample-products-page branch 2 times, most recently from 9fcee09 to 3585f40 Compare May 19, 2021 17:00
Comment on lines 34 to 35
const config = {apiKey: apiKey, shopOrigin: shop, host: host};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I failed to see this before, but the call to createApp below can also be replaced by const app = useAppBridge(); from app-bridge-react.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

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.

None yet

2 participants