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

Automattic Components: Add form label to package #79256

Merged
merged 4 commits into from
Aug 10, 2023

Conversation

jeyip
Copy link
Contributor

@jeyip jeyip commented Jul 11, 2023

To avoid creating another dependency on a non-packaged Calypso component, we want to migrate the dropdown we're using to @automattic/components

We're migrating the form label component because it is a dependency for the dropdown component, and is necessary to render said dropdown.

The entirety of the migration is a 4 step process that creates easily digestible ( and revertible ) PRs to work with. See https://github.com/Automattic/martech/issues/2028. Unfortunately, this process also eliminates the git history from migrated files unless the 4 steps are condensed into a single one. I don't have a great answer to this, but the tradeoff, to me, is worth the added safety of more understandable, compact, iterative pull requests.

Proposed Changes

Adds the form label calypso component into the @automattic/components package

Screenshots

2023-08-05 20 44 04

Testing Instructions

Note:
I initially had issues running storybook ( missing dependencies, odd runtime errors, etc. ). I had to blow away npm modules before it behaved as expected

  • At the root of the Calypso directory, run yarn storybook:start
  • Ensure that the form label component renders properly

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@jeyip jeyip self-assigned this Jul 11, 2023
@github-actions
Copy link

github-actions bot commented Jul 11, 2023

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@jeyip jeyip force-pushed the add/form-label-to-automattic-components branch from 3f6ccbb to 809f806 Compare August 6, 2023 02:52
@jeyip jeyip marked this pull request as ready for review August 6, 2023 03:42
@jeyip jeyip requested review from a team and southp August 6, 2023 03:43
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 6, 2023
Copy link
Contributor

@chriskmnds chriskmnds left a comment

Choose a reason for hiding this comment

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

Thanks again for thinking about these dependencies and resolving as you go.

  • My comment in the other PR also applies here. I don't think we should be replicating stuff without plan to deprecate and remove the original.

  • Concerning "form label", I fail to see this sitting in the root of @automattc/components. I can see there is a forms folder already, should we move and export this from there perhaps?

I can see there is a forms folder already, should we move and export this from there perhaps?

Speaking of which, I'm guessing the entirety of client/components/forms/* should gradually migrate under there. For this change, I'd also consider updating @automattc/components/forms README to match a little closer to client/components/forms/* and mention the components living under?

@jeyip
Copy link
Contributor Author

jeyip commented Aug 8, 2023

My #79257 (review) also applies here. I don't think we should be replicating stuff without plan to deprecate and remove the original.

#79257 (comment)

Concerning "form label", I fail to see this sitting in the root of @automattc/components. I can see there is a forms folder already, should we move and export this from there perhaps?

Good observation -- will do this tomorrow. 👍

@jeyip
Copy link
Contributor Author

jeyip commented Aug 8, 2023

Concerning "form label", I fail to see this sitting in the root of @automattc/components. I can see there is a forms folder already, should we move and export this from there perhaps?

Resolved in c34fa67

Speaking of which, I'm guessing the entirety of client/components/forms/* should gradually migrate under there.

I agree with this 👍

For this change, I'd also consider updating @automattc/components/forms README to match a little closer to client/components/forms/* and mention the components living under?

Resolved in 52e43fa

Form label shim is in this PR. Currently drafting up testing notes #80350

@jeyip jeyip force-pushed the add/form-label-to-automattic-components branch from 949ef6e to 52e43fa Compare August 8, 2023 23:03
@jeyip jeyip requested a review from a team August 10, 2023 05:15
@jeyip jeyip force-pushed the add/form-label-to-automattic-components branch from 52e43fa to 6528b86 Compare August 10, 2023 16:37
@jeyip jeyip force-pushed the add/form-label-to-automattic-components branch from 6528b86 to 709b630 Compare August 10, 2023 17:39
Copy link
Contributor

@ddc22 ddc22 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢
Minor ignorable comment :D

const Template = ( args ) => {
return (
<form>
<FormLabel { ...args }>Button Label</FormLabel>
Copy link
Contributor

Choose a reason for hiding this comment

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

Non essential.
Maybe add the Button Label as a dynamic arg.

Copy link
Contributor Author

@jeyip jeyip Aug 10, 2023

Choose a reason for hiding this comment

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

Hey thanks for chiming in Dehan -- I thought about doing so, but wasn't sure how to accomplish this without adding it to Default.args below.

The issue with adding it to Default.args is that the label is then shown as a "Control", which I believe mistakenly implies that it might be a prop that FormLabel accepts.

Screenshot 2023-08-10 at 2 20 15 PM

If I'm wrong about this though, definitely let me know and I'm happy to include it in a follow-up PR 🙂

@jeyip jeyip merged commit a75ebc5 into trunk Aug 10, 2023
@jeyip jeyip deleted the add/form-label-to-automattic-components branch August 10, 2023 21:30
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants