UI: Create new form elements in the new Core UI (Input, TextArea, Select) #23469

cdedreuille · 2023-07-16T21:33:27Z

What I did

As part of the work of moving components to Core UI, I'm creating 3 new components:

Input
TextArea
Select

These components are now primitives and not sub components of the Form component. For context, we are depreciating the Form component in favour of better primitives. In term of design, nothing is changing except for the fact that by default all of these component will be 100% in width.

How to test

Got to the Core UI Storybook
Test the new components

Checklist

Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
Make sure to add/update documentation regarding your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

github-actions · 2023-07-16T21:35:59Z

	Fails
🚫	PR is not labeled with one of: ["patch:no","patch:yes"]

Generated by 🚫 dangerJS against 312c23f

GadiZimerman · 2023-07-16T22:07:06Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2023-07-16T22:07:50Z

PR Analysis

🎯 Main theme: Creation of new UI form elements
📌 Type of PR: Enhancement
🧪 Relevant tests added: No
✨ Focused PR: Yes, the PR is focused as it clearly states the creation of new UI form elements (TextField, TextArea, Select) as part of the work of moving components to Core UI.
🔒 Security concerns: No, the PR does not introduce any obvious security concerns as it mainly deals with UI components.

PR Feedback

💡 General PR suggestions: The PR is well-structured and the code is clean. However, it would be beneficial to add tests for the new components to ensure they work as expected. Also, it would be good to add some error handling or input validation for the form fields.

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review - Request a review of the latest update to the PR.
/describe - Modify the PR title and description based on the contents of the PR.
/improve - Suggest improvements to the code in the PR. These will be provided as pull request comments, ready to commit.
/ask - Pose a question about the PR.

CodiumAI-Agent · 2023-07-16T22:07:52Z

code/ui/components/src/new/Input/Input.tsx

+import React, { forwardRef } from 'react';
+import { styled } from '@storybook/theming';
+
+interface InputProps {


Consider adding PropTypes for better type checking and to make the code more self-documenting. [medium]

euh ... no.

code/ui/components/src/new/Textarea/Textarea.tsx

CodiumAI-Agent · 2023-07-16T22:07:54Z

code/ui/components/src/new/Input/Input.stories.tsx

+export default meta;
+type Story = StoryObj<typeof Input>;
+
+export const Base: Story = {


Consider adding more diverse storybook examples to cover different use cases and edge cases. [medium]

Interesting one. I could do that.

socket-security · 2023-07-18T15:09:33Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages	Version	New capabilities	Transitives	Size	Publisher
@radix-ui/react-select	1.2.2	None	`+70`	2.82 MB	benoitgrelard

ndelangen · 2023-07-18T15:54:09Z

code/ui/components/src/new/Select/Select.stories.tsx

+export const Base: Story = {
+  args: {},
+  render: (_, { args }) => (
+    <div style={{ width: 400 }}>


Let's put this in a decorator, though I'm not a fan of arbitrary wrappers for widths.
I guess we don't want to use viewports here?

It's really just to test it inside a container. By default it is width 100%

ndelangen · 2023-07-18T15:55:58Z

code/ui/components/src/new/Select/Select.tsx

+export const Select = {
+  Root: SelectPrimitive.Root,
+  Group: SelectPrimitive.Group,
+  Value: SelectPrimitive.Value,
+  Trigger: SelectTrigger,
+  Content: SelectContent,
+  Label: SelectLabel,
+  Item: SelectItem,
+  Separator: SelectSeparator,
+};


For complex combination-components like these JSdoc comments with mini examples would be great.

That way when users import these, all they have to do to get a example snippet, is hover over this import.

This is just a wrapper on top of Radix with styles added to it. I can improve the docs to specify that I guess to make sure they go to Radix to understand all the use cases. We will not try to recreate their docs.

ndelangen · 2023-07-18T15:57:22Z

code/ui/components/src/new/Select/SelectItem.tsx

+interface SelectItemProps {
+  children: React.ReactNode;
+  value: string;
+}


This is a lie, it's actually much much more.

ComponentProps<typeof StyledItem>

ok. I can replace it with that type.

ndelangen · 2023-07-18T16:01:27Z

code/ui/components/src/new/Textarea/Textarea.tsx

+  return <StyledTextarea ref={ref} {...props} />;
+});
+
+Textarea.displayName = 'Textarea';


Is this needed? Why?

Because it's a forwardRef with an arrow function inside.

It is needed to make sure React devtools function correctly. This triggers an error otherwise.

ndelangen · 2023-07-18T16:03:13Z

code/ui/components/src/new/Select/SelectItem.tsx

+
+SelectItem.displayName = 'SelectItem';
+
+const StyledItem = styled(RadixSelect.Item)(({ theme }) => ({


You're not actually using the theme in this component?

ndelangen · 2023-07-18T16:03:27Z

code/ui/components/src/new/Select/SelectItem.tsx

+  },
+}));
+
+const StyledItemIndicator = styled(RadixSelect.ItemIndicator)(({ theme }) => ({


You're not actually using the theme in this component?

ndelangen · 2023-07-18T16:05:09Z

code/ui/components/src/new/Select/Select.tsx

+  overflow: 'hidden',
+  backgroundColor: theme.input.background,
+  borderRadius: '6px',
+  border: theme.base === 'dark' ? `1px solid ${theme.input.border}` : 'none',


Interesting choice to have no border at all in dark-mode. Can you elaborate?
Would a 1px solid transparent not be better? Less layout shifts.

You can't have any layout shift since it will close the panel when you click anywhere else but you're right just to be safe we can add 1px solid transparent

ndelangen · 2023-07-18T16:07:09Z

code/ui/components/src/new/Input/Input.tsx

+import React, { forwardRef } from 'react';
+import { styled } from '@storybook/theming';
+
+interface InputProps {


This is a lie, it's actually more!

ComponentProps<typeof StyledInput>

ndelangen · 2023-07-18T16:12:47Z

code/ui/components/src/new/Input/Input.tsx

+export const Input = forwardRef<HTMLInputElement, InputProps>(({ ...props }, ref) => {
+  return <StyledInput ref={ref} {...props} />;
+});


{ ...props } does not make any sense

Create new TextField component

312c23f

cdedreuille added ui maintenance User-facing maintenance tasks ci:normal labels Jul 16, 2023

cdedreuille changed the base branch from next to release/7.2 July 16, 2023 21:33

cdedreuille added 2 commits July 16, 2023 22:55

Add Textarea component

f56ded7

Rename TextField component into Input

f9b9e9a

cdedreuille added the patch:no label Jul 16, 2023

CodiumAI-Agent reviewed Jul 16, 2023

View reviewed changes

code/ui/components/src/new/Textarea/Textarea.tsx Show resolved Hide resolved

CodiumAI-Agent reviewed Jul 16, 2023

View reviewed changes

cdedreuille changed the title ~~UI: Create new form elements in the new Core UI (TextField, TextArea, Select)~~ UI: Create new form elements in the new Core UI (Input, TextArea, Select) Jul 16, 2023

cdedreuille added 3 commits July 17, 2023 13:02

First pass at creating a new Select component

be6ad7b

Improve styling + api of the Select component

42ba728

Update Select.stories.tsx

4d2bcad

cdedreuille marked this pull request as ready for review July 18, 2023 14:57

cdedreuille added 2 commits July 18, 2023 16:01

Merge branch 'next' into charles/update-form-ui

d3dad50

Merge branch 'release/7.2' into charles/update-form-ui

4a21186

cdedreuille requested review from ndelangen and JReinhold July 18, 2023 15:04

ndelangen reviewed Jul 18, 2023

View reviewed changes

Some cleaning

a164fd1

cdedreuille merged commit 53276b5 into release/7.2 Jul 18, 2023

cdedreuille deleted the charles/update-form-ui branch July 18, 2023 17:40

vanessayuenn assigned cdedreuille Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: Create new form elements in the new Core UI (Input, TextArea, Select) #23469

UI: Create new form elements in the new Core UI (Input, TextArea, Select) #23469

cdedreuille commented Jul 16, 2023 •

edited

Loading

github-actions bot commented Jul 16, 2023

GadiZimerman commented Jul 16, 2023

CodiumAI-Agent commented Jul 16, 2023

CodiumAI-Agent Jul 16, 2023

cdedreuille Jul 16, 2023

CodiumAI-Agent Jul 16, 2023

cdedreuille Jul 16, 2023

socket-security bot commented Jul 18, 2023

ndelangen Jul 18, 2023

cdedreuille Jul 18, 2023

ndelangen Jul 18, 2023

cdedreuille Jul 18, 2023

ndelangen Jul 18, 2023

cdedreuille Jul 18, 2023

ndelangen Jul 18, 2023

cdedreuille Jul 18, 2023

cdedreuille Jul 18, 2023

ndelangen Jul 18, 2023

ndelangen Jul 18, 2023

ndelangen Jul 18, 2023

cdedreuille Jul 18, 2023

ndelangen Jul 18, 2023

ndelangen Jul 18, 2023

cdedreuille Jul 18, 2023


		SelectItem.displayName = 'SelectItem';

		const StyledItem = styled(RadixSelect.Item)(({ theme }) => ({

UI: Create new form elements in the new Core UI (Input, TextArea, Select) #23469

UI: Create new form elements in the new Core UI (Input, TextArea, Select) #23469

Conversation

cdedreuille commented Jul 16, 2023 • edited Loading

What I did

How to test

Checklist

Maintainers

github-actions bot commented Jul 16, 2023

GadiZimerman commented Jul 16, 2023

CodiumAI-Agent commented Jul 16, 2023

PR Analysis

PR Feedback

How to use

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

socket-security bot commented Jul 18, 2023

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

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

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

Choose a reason for hiding this comment

cdedreuille commented Jul 16, 2023 •

edited

Loading