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

Update product-configuration template #73

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

siddhantbajaj
Copy link
Contributor

Background

(Provide a link to any relevant issues AND provide a TLDR of the issue in this pull request)

Solution

Before:

Screenshot 2023-12-07 at 9 36 09 AM

After:
Screenshot 2023-12-07 at 9 33 22 AM

Checklist

  • I have 🎩'd these changes
  • I have squashed my commits into chunks of work with meaningful commit messages

Copy link
Contributor

@carrotderek carrotderek left a 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'>();
Copy link
Contributor

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?

Copy link
Contributor

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?

Copy link
Contributor

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)

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, I missed commenting that out.

Comment on lines 30 to 46
<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>
Copy link
Contributor

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.

Copy link
Contributor

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.

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 flavor contains "typescript" %}
const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>();
// const {extension: {target}, i18n} = useApi<'admin.product-details.configuration.render'>();
Copy link
Contributor

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?

Comment on lines 30 to 46
<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>
Copy link
Contributor

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.

Comment on lines 31 to 45
{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}`} />
])}
Copy link
Contributor

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

Suggested change
{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>
)}

Copy link
Contributor Author

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'>();
Copy link
Contributor

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)

Copy link
Contributor

@carrotderek carrotderek left a 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');
Copy link
Contributor

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.

Suggested change
import React = require('react');
import React from 'react';

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.

3 participants