feat(detail-pages): add <{Info,Form,CustomForm}DetailPage> components #2594

kark · 2022-05-06T15:01:56Z

Summary

The aim of this PR was to implement new <InfoDetailPage>, <FormDetailPage> and <CustomFormDetailPage> components.
closes #2551

vercel · 2022-05-06T15:02:00Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
merchant-center-application-kit	✅ Ready (Inspect)	Visit Preview	May 10, 2022 at 2:14PM (UTC)

changeset-bot · 2022-05-06T15:02:00Z

🦋 Changeset detected

Latest commit: 120bc42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
@commercetools-frontend/application-components	Minor
@commercetools-website/custom-applications	Minor
@commercetools-website/components-playground	Minor
@commercetools-frontend/application-shell	Minor
merchant-center-application-template-starter	Patch
playground	Patch
@commercetools-local/visual-testing-app	Patch
@commercetools-frontend/cypress	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

docs: improvments

docs: improvements #2

revert: defaultProps

emmenko

Thanks for working on this 🙌

Had a quick look with some questions. Will have another look later.

emmenko · 2022-05-10T09:04:23Z

.changeset/old-otters-arrive.md

+
+## InfoDetailPage
+
+Form Detail pages are controlled components used to render a page with detailed data.


Suggested change

Form Detail pages are controlled components used to render a page with detailed data.

Info Detail pages are controlled components used to render a page with detailed data.

emmenko · 2022-05-10T09:12:04Z

...n-components/src/components/detail-pages/custom-form-detail-page/custom-form-detail-page.tsx

+  /**
+   * Makes page top bar visible
+   */
+  showPageTopBar?: boolean;


Is this a new prop? What's the purpose of it? 🤔

Thanks. Looking at what is expected at /customers/new in the MC:

the top bar has to be optional.

The other though I had was to render it conditionally based on the onPreviousPathClick

The other though I had was to render it conditionally based on the onPreviousPathClick

Yeah maybe that's better so we don't need an extra prop for that.

packages/application-components/src/index.ts

CarlosCortizasCT

Awesome work 💪

I just left a comment with something to maybe discuss about.

CarlosCortizasCT · 2022-05-10T10:00:42Z

.changeset/old-otters-arrive.md

+    <FormDetailPage
+      title="Manage your account"
+      showPageTopBar
+      onPreviousPathClick={() => history.push('/starting-page')}
+      isPrimaryButtonDisabled={formik.isSubmitting}
+      onSecondaryButtonClick={() => {
+        formik.resetForm();
+      }}
+      onPrimaryButtonClick={formik.handleSubmit}
+    >
+      <TextField
+        name="email"
+        title="Email"
+        isRequired={true}
+        value={formik.values.email}
+        errors={formik.errors.email}
+        touched={formik.touched.email}
+        onChange={formik.handleChange}
+        onBlur={formik.handleBlur}
+      />
+    </FormDetailPage>


For both FormDetailPage and CustomFormDetailPage I'm missing the form html tag wrapping the input fields.

Personally, I would expect those components to internally add that element but, if we don't want that solution, at least I find it necessary to add it into the examples (both in the changelog and in the docs).

If we include the form tag in our components, perhaps we would also want to link the primary button to it using the form attribute (HTML attributes; form attribute explained). Although this can be tricky with the current implementation.

I pushed a branch (cm-details-pages) with an implementation idea (not a working solution) of what I meant with my previous comment in case we want to discuss adding the form tag to the new form components.

Changes can be seen here.

FYI: I think using the form element is a valid point and we should discuss this separately, as we're also not really using it in many parts of the MC at the moment.

@CarlosCortizasCT Can you maybe open a discussion or issue to talk about it and how we plan to use it? We should also check with the other teams if there are other concerns about having it set by default in the components.

Thanks

I'd also value this as I know many locations inside the MC where it's not used. Let's discuss it separately. I think we can either scan the next child and if it's a form not wrap it or have a disableWrapByForm prop which is false by default but can be disabled for instance. Similar to Next.js Link migration strategy which has a oldBehaviour prop.

CarlosCortizasCT

💯

tdeekens

Super cool. Can't wait to get rid of more homegrown internal things inside the MC with this 💟.

tdeekens · 2022-05-11T09:01:47Z

.changeset/old-otters-arrive.md

+'@commercetools-website/components-playground': minor
+---
+
+Add three new layout components: `<InfoDetailPage>`, `<FormDetailPage>` and `<CustomFormDetailPage>`.


Suggested change

Add three new layout components: `<InfoDetailPage>`, `<FormDetailPage>` and `<CustomFormDetailPage>`.

Adds three new layout components: `<InfoDetailPage>`, `<FormDetailPage>` and `<CustomFormDetailPage>`.

tdeekens · 2022-05-11T09:03:44Z

.changeset/old-otters-arrive.md

+            onClick={() => {
+              formik.resetForm();
+            }}


Nit as we do that below

Suggested change

onClick={() => {

formik.resetForm();

}}

onClick={formik.resetForm}

tdeekens · 2022-05-11T09:06:35Z

visual-testing-app/src/components/info-detail-page/info-detail-page.visualroute.tsx

@@ -0,0 +1,59 @@
+import Text from '@commercetools-uikit/text';


GitHub doesn't have syntax highlighting on this file but I guess that's unrelated to the file itself.

tdeekens · 2022-05-11T09:07:14Z

visual-testing-app/src/components/info-detail-page/info-detail-page.visualroute.tsx

+    <Spec label="InfoDetailPage" size="xl">
+      <DetailPageContainer onPreviousPathClick={() => alert('Go back clicked')}>
+        <Text.Body>
+          {`Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur nec turpis in risus elementum fringilla. Vestibulum nec vulputate metus, fringilla luctus nisl. Vestibulum mattis ultricies augue sagittis vestibulum. Nulla facilisi. Quisque tempor pulvinar efficitur. Praesent interdum ultrices leo. Vivamus non ex maximus justo egestas suscipit eget sed purus. Aliquam ut venenatis nulla. Fusce ac ligula viverra, blandit augue eget, congue turpis. Curabitur a sagittis leo. Nunc sed quam dictum, placerat nunc quis, luctus erat.`}


I don't think we need tow rap these in {``}. There are some places in this PR we do so and others we don't.

tdeekens

Thanks. This looks good from my view.

github-actions · 2022-05-11T11:50:40Z

Deploy preview for merchant-center-application-kit ready!

✅ Preview
https://merchant-center-application-d6uavsnfk-commercetools.vercel.app
https://appkit-sha-3b70baebb8e5a69920b1196094bef199e801d096.commercetools.vercel.app
https://appkit-pr-2594.commercetools.vercel.app

Built with commit 120bc42.
This pull request is being automatically deployed with vercel-action

emmenko · 2022-05-13T15:50:01Z

@kark Looks good, thanks!

One minor thing: the list of components got longer now and I think it would be nice to group things a bit (e.g. Dialogs, Modal pages, Detail pages, etc.)

Can also be a follow up.

emmenko

🚀

Let's do the docs improvements as a follow up, as discussed. Thanks!

emmenko · 2022-05-12T12:15:53Z

website-components-playground/src/pages/custom-form-detail-page.js

-                      onClick={() => {
-                        formikProps.resetForm();
-                      }}


I would keep it like this actually, as resetForm accepts an argument as the nextState and that could lead to unexpected issues if the function is called with the event resetForm(event).

https://formik.org/docs/api/formik#resetform-nextstate-partialformikstatevalues--void

Thanks 😊

kark added the 🚧 Status: WIP Work in progress label May 6, 2022

vercel bot temporarily deployed to Preview May 6, 2022 15:02 Inactive

feat(detail-pages): add <{Info,Form,CustomForm}DetailPage> components

15cc4cb

docs: improvments

kark force-pushed the kk-detail-pages branch from b8c2a16 to 15cc4cb Compare May 9, 2022 12:57

vercel bot temporarily deployed to Preview May 9, 2022 13:00 Inactive

Merge branch 'main' into kk-detail-pages

74eef50

vercel bot temporarily deployed to Preview May 9, 2022 13:35 Inactive

docs: improvements

bf377cd

docs: improvements #2

kark force-pushed the kk-detail-pages branch from 0cc747a to bf377cd Compare May 9, 2022 14:07

vercel bot temporarily deployed to Preview May 9, 2022 14:11 Inactive

Merge branch 'main' into kk-detail-pages

a1ed80f

vercel bot temporarily deployed to Preview May 9, 2022 15:17 Inactive

vercel bot temporarily deployed to Preview May 9, 2022 15:33 Inactive

chore: typo in changeset

50c6a17

revert: defaultProps

kark force-pushed the kk-detail-pages branch from a0290eb to 50c6a17 Compare May 9, 2022 15:41

vercel bot temporarily deployed to Preview May 9, 2022 15:47 Inactive

kark added 2 commits May 10, 2022 09:56

refactor(detail-pages): show top bar by default

f35c6ca

Merge branch 'main' into kk-detail-pages

32bb981

vercel bot temporarily deployed to Preview May 10, 2022 08:06 Inactive

kark marked this pull request as ready for review May 10, 2022 08:28

kark requested a review from a team May 10, 2022 08:28

kark removed the 🚧 Status: WIP Work in progress label May 10, 2022

emmenko reviewed May 10, 2022

View reviewed changes

refactor(detail-pages): move tabular detail page to detail pages folder

90c3558

vercel bot temporarily deployed to Preview May 10, 2022 09:59 Inactive

CarlosCortizasCT reviewed May 10, 2022

View reviewed changes

docs: remove showPageTopBar prop

d9c2522

CarlosCortizasCT approved these changes May 10, 2022

View reviewed changes

vercel bot temporarily deployed to Preview May 10, 2022 14:14 Inactive

kark requested a review from emmenko May 11, 2022 06:09

tdeekens reviewed May 11, 2022

View reviewed changes

refactor: code review improvements

e9a8d0e

tdeekens approved these changes May 11, 2022

View reviewed changes

Merge branch 'main' into kk-detail-pages

3c35c85

github-actions bot temporarily deployed to Preview May 11, 2022 11:49 Destroyed

kark added 2 commits May 12, 2022 17:30

style: change layout of detail pages

f9248f1

Merge branch 'main' into kk-detail-pages

2a87715

github-actions bot temporarily deployed to Preview May 12, 2022 15:36 Destroyed

refactor: make onPreviousPathClick prop required

7767284

github-actions bot temporarily deployed to Preview May 13, 2022 12:02 Destroyed

emmenko approved these changes May 16, 2022

View reviewed changes

refactor: revert reset form function

120bc42

github-actions bot temporarily deployed to Preview May 16, 2022 13:18 Destroyed

emmenko merged commit d20638e into main May 16, 2022

emmenko deleted the kk-detail-pages branch May 16, 2022 13:38

ghost mentioned this pull request May 16, 2022

Version Packages #2591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(detail-pages): add <{Info,Form,CustomForm}DetailPage> components #2594

feat(detail-pages): add <{Info,Form,CustomForm}DetailPage> components #2594

kark commented May 6, 2022

vercel bot commented May 6, 2022 •

edited

Loading

changeset-bot bot commented May 6, 2022 •

edited

Loading

emmenko left a comment

emmenko May 10, 2022

emmenko May 10, 2022

kark May 10, 2022 •

edited

Loading

emmenko May 10, 2022

CarlosCortizasCT left a comment

CarlosCortizasCT May 10, 2022

CarlosCortizasCT May 10, 2022

emmenko May 10, 2022

tdeekens May 11, 2022

CarlosCortizasCT left a comment

tdeekens left a comment

tdeekens May 11, 2022

tdeekens May 11, 2022

tdeekens May 11, 2022

tdeekens May 11, 2022

tdeekens left a comment

github-actions bot commented May 11, 2022 •

edited

Loading

emmenko commented May 13, 2022

emmenko left a comment

emmenko May 12, 2022

kark May 16, 2022


		## InfoDetailPage

		Form Detail pages are controlled components used to render a page with detailed data.

	Form Detail pages are controlled components used to render a page with detailed data.
	Info Detail pages are controlled components used to render a page with detailed data.

	Add three new layout components: `<InfoDetailPage>`, `<FormDetailPage>` and `<CustomFormDetailPage>`.
	Adds three new layout components: `<InfoDetailPage>`, `<FormDetailPage>` and `<CustomFormDetailPage>`.

		@@ -0,0 +1,59 @@
		import Text from '@commercetools-uikit/text';

feat(detail-pages): add <{Info,Form,CustomForm}DetailPage> components #2594

feat(detail-pages): add <{Info,Form,CustomForm}DetailPage> components #2594

Conversation

kark commented May 6, 2022

Summary

vercel bot commented May 6, 2022 • edited Loading

changeset-bot bot commented May 6, 2022 • edited Loading

🦋 Changeset detected

emmenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kark May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosCortizasCT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosCortizasCT left a comment

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

github-actions bot commented May 11, 2022 • edited Loading

emmenko commented May 13, 2022

emmenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented May 6, 2022 •

edited

Loading

changeset-bot bot commented May 6, 2022 •

edited

Loading

kark May 10, 2022 •

edited

Loading

github-actions bot commented May 11, 2022 •

edited

Loading