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

Components: Integrate TextInput component [WIP] #32381

Closed
wants to merge 4 commits into from

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jun 2, 2021

🚧 WIP

Description

This PR integrates the g2 TextInput component.

Integration tracking issue: #30503

Related: #28399

How has this been tested?

This is a new component.

We'll check that:

  • storybook story works ❓ There are a lot Typescript errors, some from other sources that prevent me from running npm run storybook:dev so I've removed the import from tsconfig.json for now.
  • unit tests pass
 npm run test-unit packages/components/src/utils/test/values.js
 npm run test-unit packages/components/src/text-input/test

Screenshots

To come...

Types of changes

A new feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@@ -40,6 +40,7 @@ export default {
controlPaddingXLarge: `calc(${ CONTROL_PADDING_X } * 1.3334)`,
controlPaddingXSmall: `calc(${ CONTROL_PADDING_X } / 1.3334)`,
controlBackgroundColor: COLORS.white,
controlBackgroundColorHover: 'rgba(0, 0, 0, 0.5)',
Copy link
Member Author

Choose a reason for hiding this comment

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

I took this straight from G2's controlBackgroundColorHover.

What's the best way to migrate these styles and know which is the default?

Or are we going to clean up/make things consistent across the board later?

@@ -50,6 +51,8 @@ export default {
controlHeightLarge: `calc( ${ CONTROL_HEIGHT } * 1.2 )`,
controlHeightSmall: `calc( ${ CONTROL_HEIGHT } * 0.8 )`,
controlHeightXSmall: `calc( ${ CONTROL_HEIGHT } * 0.6 )`,
controlInnerControlTextColor: COLORS.darkGray,
controlTextColor: COLORS.darkGray,
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above. These are fairy arbitrary and are probably wrong, or belong in COLORS.

I'm not aware of any system to follow 🤷

@ramonjd ramonjd force-pushed the add/g2-component-text-input branch from 462bd8c to 6fac8fd Compare June 3, 2021 09:40
@ciampo
Copy link
Contributor

ciampo commented Jun 3, 2021

Hey @ramonjd, thank you for working on this PR!

I took a look at this PR, and it looks like there are still quite a few build errors that prevent you from running the Storybook story correctly. I tinkered around on my local machine and came up with these fixes:

  1. Rebased on top of latest trunk in order to include Storybook: fix misc warnings #32401
  2. Fixed the Icon import in text-input-arrows.js
  3. Ran some combination of npm run distclean && npm i in order to include react-autosize-textarea and react-number-format in the package-lock.json file
  4. Interpolated some CONFIG variables in ${…} in order not to break styles.js at runtime
  5. Added the text-input folder to tsconfig.json, so that it gets checked by TypeScript
Since I don't seem to have write rights on your PR branch, I've prepared a diff including the code changes from points 2 to 5 (expand to reveal)
diff --git a/package-lock.json b/package-lock.json
index 5a52107892..e450e3268b 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -13224,7 +13224,9 @@
 				"memize": "^1.1.0",
 				"moment": "^2.22.1",
 				"re-resizable": "^6.4.0",
+				"react-autosize-textarea": "^7.1.0",
 				"react-dates": "^17.1.1",
+				"react-number-format": "^4.6.0",
 				"react-resize-aware": "^3.1.0",
 				"react-spring": "^8.0.20",
 				"react-use-gesture": "^9.0.0",
@@ -51265,6 +51267,14 @@
 				"prop-types": "^15.5.10"
 			}
 		},
+		"react-number-format": {
+			"version": "4.6.0",
+			"resolved": "https://registry.npmjs.org/react-number-format/-/react-number-format-4.6.0.tgz",
+			"integrity": "sha512-0Td7QM3FGFVOw+DzYooLae50d6r67hZcinbDy5TQRQJB9+/p1caX+NQQ6PW408H+nZTKgykknQHv6L6PPoRO2w==",
+			"requires": {
+				"prop-types": "^15.7.2"
+			}
+		},
 		"react-outside-click-handler": {
 			"version": "1.3.0",
 			"resolved": "https://registry.npmjs.org/react-outside-click-handler/-/react-outside-click-handler-1.3.0.tgz",
diff --git a/packages/components/src/text-input/styles.js b/packages/components/src/text-input/styles.js
index 1e690a4b54..b302a7cc1a 100644
--- a/packages/components/src/text-input/styles.js
+++ b/packages/components/src/text-input/styles.js
@@ -26,7 +26,7 @@ export const inputMultiline = css`
 `;
 
 export const inputFontSize = css`
-	font-size: CONFIG.fontSizeInputMobile;
+	font-size: ${ CONFIG.fontSizeInputMobile };
 	@media ( min-width: 36em ) {
 		font-size: ${ CONFIG.fontSize };
 	}
@@ -36,10 +36,10 @@ export const Input = css`
 	appearance: none;
 	background: transparent;
 	border: none;
-	border-radius: CONFIG.controlBorderRadius;
+	border-radius: ${ CONFIG.controlBorderRadius };
 	box-shadow: none;
 	box-sizing: border-box;
-	color: CONFIG.controlTextColor;
+	color: ${ CONFIG.controlTextColor };
 	display: block;
 	flex: 1;
 	line-height: 18px;
diff --git a/packages/components/src/text-input/text-input-arrows.js b/packages/components/src/text-input/text-input-arrows.js
index 5ba2dd8dc0..90b4b8a0c7 100644
--- a/packages/components/src/text-input/text-input-arrows.js
+++ b/packages/components/src/text-input/text-input-arrows.js
@@ -18,7 +18,7 @@ import {
 /**
  * Internal dependencies
  */
-import { Icon } from '../icon';
+import Icon from '../icon';
 import { View } from '../view';
 import { VStack } from '../v-stack';
 import * as styles from './styles';
diff --git a/packages/components/tsconfig.json b/packages/components/tsconfig.json
index 188a594d27..65df915199 100644
--- a/packages/components/tsconfig.json
+++ b/packages/components/tsconfig.json
@@ -33,6 +33,7 @@
 		"src/shortcut/**/*",
 		"src/spinner/**/*",
 		"src/spacer/**/*",
+		"src/text-input/**/*",
 		"src/tip/**/*",
 		"src/truncate/**/*",
 		"src/ui/**/*",

I recommend running npm run distclean && npm run ci after applying the changes above.

With these fixes in place, the npm run storybook:dev command should run successfully — but the TextInput story will still not appear (or error when forced to). There seem to be some issues with the useTextInput() hook — I commented it out and the errors were gone.

Unfortunately, I didn't have enough time to go to the bottom of this, but I noticed that the types.ts file is showing some errors — that could be a good start to understand what's causing the errors!

Another good resource is this short guide by @sarayourfriend on migrating g2 components to gutenberg.

@ramonjd
Copy link
Member Author

ramonjd commented Jun 3, 2021

@ciampo Thanks so much for looking at this, and sorry I left so many silly errors in there. I had fixed some of the more basic ones like imports and interpolation locally but, as always, forgot to push 🤦

I appreciate your help, and will follow your advice above to keep digging away 🙏

@ramonjd ramonjd force-pushed the add/g2-component-text-input branch 2 times, most recently from 5846389 to 6df54c9 Compare June 4, 2021 06:34
@ramonjd
Copy link
Member Author

ramonjd commented Jun 4, 2021

I might chip away slowly at this.

There are a bunch of Typescript errors that run deep and/or across files, and whose priorities I'm not quite sure.

Whenever @sarayourfriend is next available to lend some 👀 I'd be sure grateful for your advice too! Thanks!

@ramonjd ramonjd force-pushed the add/g2-component-text-input branch from c5fc215 to 0e56556 Compare June 4, 2021 07:02
@ciampo
Copy link
Contributor

ciampo commented Jun 4, 2021

I appreciate your help, and will follow your advice above to keep digging away

Don't mention it! And feel free to ping me again :)

@ramonjd ramonjd force-pushed the add/g2-component-text-input branch from e550180 to 183a4b9 Compare June 6, 2021 05:49
packages/components/src/text-input/README.md Outdated Show resolved Hide resolved
packages/components/src/text-input/README.md Outdated Show resolved Hide resolved
packages/components/src/utils/events.js Show resolved Hide resolved
packages/components/src/utils/hooks/index.js Show resolved Hide resolved
packages/components/src/utils/keyboard.js Show resolved Hide resolved
packages/components/src/utils/values.js Outdated Show resolved Hide resolved
@ramonjd
Copy link
Member Author

ramonjd commented Jun 16, 2021

Thanks so much for the detailed review @sarayourfriend ❤️ . Really helpful.

Yes, this is a big one! :) My eyes were too big for my 🧠, but I'm learning a whole bunch along the way.

I'll slowly chip away where I can.

There are some type errors I've ignored, mostly related to PolymorphicComponentProps, which are cryptic to me so I might ping later to see if we need to do anything about it now.

Also I can look into moving those utils into packages if you think it's okay. Would it make more sense to do it in separate PRs?

@sarayourfriend
Copy link
Contributor

Also I can look into moving those utils into packages if you think it's okay. Would it make more sense to do it in separate PRs?

I think based on @gziolo's answer we can ignore those questions of mine 🙂

Feel free to ping with any questions regarding the types. They can be pretty non-obvious to work with... 😞

ramonjd added 4 commits June 18, 2021 16:55
Reformatting
Guessing styles and config
…ts. Some utils could be exported from /utils/index.js

Adding stories
Exporting TextInput in components/index.js

Formatting steppers
Trying to get the story to appear in storybook
Added 2 x dependencies: react-autosize-textarea and react-number-format.

Formatting tests

Updated snapshot

Adding type annotations

Updating type declarations across TextInput
Updating snapshots
Editing README

Fixing types paths
@ramonjd ramonjd force-pushed the add/g2-component-text-input branch from ebd65b5 to 27f6bfc Compare June 18, 2021 06:55
@skorasaurus skorasaurus added the [Package] Components /packages/components label Aug 19, 2021
@ramonjd ramonjd closed this Mar 24, 2022
@ramonjd ramonjd deleted the add/g2-component-text-input branch March 24, 2022 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants