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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add FormSection component #34348

Merged
merged 6 commits into from
Aug 18, 2022
Merged

Add FormSection component #34348

merged 6 commits into from
Aug 18, 2022

Conversation

joshuatf
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Adds a FormSection component for use in the product management layouts.

How to test the changes in this Pull Request:

  1. Run npm run storybook
  2. See the FormSection component

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@joshuatf joshuatf requested a review from a team August 16, 2022 19:25
@joshuatf joshuatf self-assigned this Aug 16, 2022
@github-actions github-actions bot added the package: @woocommerce/components issues related to @woocommerce/components label Aug 16, 2022
joshuatf
joshuatf commented Aug 16, 2022
View reviewed changes
packages/js/components/src/form-section/stories/index.tsx
title="My form section"
description="Some text to describe what this section covers"
>
<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.

@louwie17 I removed the Card styles and rely directly on the Card component since some sections contain multiple cards within the content area, but let me know if you have any issue with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I always forget about the Card component, relying on that makes much more sense.

louwie17
louwie17 previously approved these changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Nice work @joshuatf, simple and straight forward.
I think this is good to merge as is, with maybe a few iterations to come, referring to Slack and the possible addition of a link underneath the description and allowing for other items in the title as this screenshot shows:
image

Maybe we do want to follow the same format WCpay was using to accommodate for this by just passing a title component in ( https://github.com/Automattic/woocommerce-payments/blob/cc6fa05c83cf38833c2e79bb8ee9af7be6c75659/client/settings/settings-section.js#L14 )
Otherwise I was thinking we could create a wrapper FormSectionTitle component and pass that into the children, then filtering that out into the header section.
Similar to what Jacob did with the Split button approach.

packages/js/components/src/form-section/stories/index.tsx
title="My form section"
description="Some text to describe what this section covers"
>
<Card>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I always forget about the Card component, relying on that makes much more sense.

@louwie17
Copy link
Contributor

Btw I think a rebase should fix the Highlight templates and hooks changes GH action as I believe a fix was merged: #34349

@joshuatf
Copy link
Contributor Author

@louwie17 Rebased and also updated the description/title to allow JSX elements and make this a bit more flexible.
Screen Shot 2022-08-17 at 2 18 41 PM

@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2022 •
edited

Test Results Summary

Commit SHA: 4a991d6

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests11800201200m 40s
E2E Tests186001018714m 40s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

louwie17
louwie17 approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

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

Rebased and also updated the description/title to allow JSX elements and make this a bit more flexible.

Awesome, thanks @joshuatf, this looks great!! LGTM 🚀

@joshuatf joshuatf merged commit e73ffbe into trunk Aug 18, 2022
@joshuatf joshuatf deleted the add/section-component branch August 18, 2022 17:36
rcstr added a commit that referenced this pull request Aug 19, 2022
@rcstr
Merge branch 'trunk' into fix/connect-command-text
defc0a3 
* trunk:
  Feature/review table list (#34393)
  Update Information Notice for No Products Found (#34362)
  Add product link field (#34313)
  Link backfilling to migration process (#34167)
  Restore 'Contributing to WooCommerce' guidance (#34380)
  Add FormSection component (#34348)
  Add SearchControl component (#34159)
  Update in-app marketplace to display localized strings (#34356)
  Update "Refund Returns" note to display localized strings (#34352)
  Added new command to set up the local environment with COT enabled. (#34321)
  Exclude asterisk from Country / Region translation string (#34368)
  add: store address tour tracks (#34337)
  Update inbox notes to display localized strings when locale changed (#34038)
  6.8 changelog: Copy from release branch into trunk (#34365)
  add tests for shipping zone regions (#34339)
@octaedro octaedro mentioned this pull request Aug 23, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@louwie17 louwie17 louwie17 approved these changes

Assignees

@joshuatf joshuatf

Labels
package: @woocommerce/components issues related to @woocommerce/components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@joshuatf @louwie17