-
Notifications
You must be signed in to change notification settings - Fork 8
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
Update product-configuration template #73
base: main
Are you sure you want to change the base?
Conversation
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 start to this. Mainly question around whether we can simplify the markup by using the ResourceList
to render the bundle components.
{% if flavor contains "typescript" %} | ||
const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>(); | ||
// const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>(); |
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.
Do we need to comment this out?
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.
linter might complain about unused variables?
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.
hmm... if we leave this commented out, we ALSO need to comment out 2 lines below (the non-typescript version)
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.
Yes, I missed commenting that out.
<BlockStack gap="small"> | ||
{bundleComponents.flatMap((component, index) => [ | ||
<InlineStack gap key={component}> | ||
<Box padding="none" inlineSize="10%"> | ||
<Icon name="ImageMajor" size="fill" /> | ||
</Box> | ||
<Box padding="large none"> | ||
<BlockStack gap="none" padding="none"> | ||
<Text fontWeight="bold-200"> | ||
{component} | ||
</Text> | ||
</BlockStack> | ||
</Box> | ||
</InlineStack>, | ||
index < bundleComponents.length - 1 && <Divider key={`divider-${index}`} /> | ||
])} | ||
</BlockStack> |
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.
Can we use <ResourceList />
and <ResourceItem />
components? It would simplify the markup here and I think they are available in the ui-extensions-react
library.
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.
yeah, they are available, and are what we actually have in the standard card.
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.
Doesn't look like it exists there - https://shopify.slack.com/archives/C04K7BZDH3N/p1702483564729509?thread_ts=1702483485.885719&cid=C04K7BZDH3N
{% if flavor contains "typescript" %} | ||
const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>(); | ||
// const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>(); |
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.
linter might complain about unused variables?
<BlockStack gap="small"> | ||
{bundleComponents.flatMap((component, index) => [ | ||
<InlineStack gap key={component}> | ||
<Box padding="none" inlineSize="10%"> | ||
<Icon name="ImageMajor" size="fill" /> | ||
</Box> | ||
<Box padding="large none"> | ||
<BlockStack gap="none" padding="none"> | ||
<Text fontWeight="bold-200"> | ||
{component} | ||
</Text> | ||
</BlockStack> | ||
</Box> | ||
</InlineStack>, | ||
index < bundleComponents.length - 1 && <Divider key={`divider-${index}`} /> | ||
])} | ||
</BlockStack> |
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.
yeah, they are available, and are what we actually have in the standard card.
{bundleComponents.flatMap((component, index) => [ | ||
<InlineStack gap key={component}> | ||
<Box padding="none" inlineSize="10%"> | ||
<Icon name="ImageMajor" size="fill" /> | ||
</Box> | ||
<Box padding="large none"> | ||
<BlockStack gap="none" padding="none"> | ||
<Text fontWeight="bold-200"> | ||
{component} | ||
</Text> | ||
</BlockStack> | ||
</Box> | ||
</InlineStack>, | ||
index < bundleComponents.length - 1 && <Divider key={`divider-${index}`} /> | ||
])} |
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.
instead of using flatMap
(which works :D) it is more common to use map
with a Fragment
{bundleComponents.flatMap((component, index) => [ | |
<InlineStack gap key={component}> | |
<Box padding="none" inlineSize="10%"> | |
<Icon name="ImageMajor" size="fill" /> | |
</Box> | |
<Box padding="large none"> | |
<BlockStack gap="none" padding="none"> | |
<Text fontWeight="bold-200"> | |
{component} | |
</Text> | |
</BlockStack> | |
</Box> | |
</InlineStack>, | |
index < bundleComponents.length - 1 && <Divider key={`divider-${index}`} /> | |
])} | |
{bundleComponents.map((component, index) => | |
<React.Fragment key={component}> | |
<InlineStack gap> | |
<Box padding="none" inlineSize="10%"> | |
<Icon name="ImageMajor" size="fill" /> | |
</Box> | |
<Box padding="large none"> | |
<BlockStack gap="none" padding="none"> | |
<Text fontWeight="bold-200"> | |
{component} | |
</Text> | |
</BlockStack> | |
</Box> | |
</InlineStack> | |
{index < bundleComponents.length - 1 && <Divider />} | |
</React.Fragment> | |
)} |
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.
Updated. I also had to add an import for react:
import React = require('react');
Let me know If I'm doing something wrong.
{% if flavor contains "typescript" %} | ||
const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>(); | ||
// const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>(); |
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.
hmm... if we leave this commented out, we ALSO need to comment out 2 lines below (the non-typescript version)
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.
Looks like we're going to have to without ResourceList
then 😆 - thanks for tracking that down.
Just need to fix the import statement for React and we should be good.
@@ -1,39 +1,87 @@ | |||
{%- if flavor contains "react" -%} | |||
import React = require('react'); |
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.
ES modules is the standard when working within the browser.
import React = require('react'); | |
import React from 'react'; |
Background
(Provide a link to any relevant issues AND provide a TLDR of the issue in this pull request)
Solution
Before:
After:
Checklist