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

replace text input by texterarea #3473

Merged
merged 8 commits into from
Jan 18, 2024
Merged

replace text input by texterarea #3473

merged 8 commits into from
Jan 18, 2024

Conversation

jeet1desai
Copy link
Contributor

@jeet1desai jeet1desai commented Jan 16, 2024

Fixes #3472

replace.text.input.webm

@Bonapara
Copy link
Member

Bonapara commented Jan 16, 2024

Hi @jeet1desai! Thank you for your contribution! It's nearly perfect. Could you please make the box shadow expand along with the input?

Also, can you add an overflow when the input is closed to prevent its content from overflowing on the right?

@lucasbordeau
Copy link
Contributor

Hi, here are some technical guidelines :

  • You'll have to see how to handle FieldInputOverlay that should grow with your textarea, the height is currently fixed at 32px.
  • Please use our generic OverflowingTextWithTooltip to handle overflowing texts, this requires fiddling a bit with the overflow properties of the parent components.

@jeet1desai
Copy link
Contributor Author

overflow.and.replace.text.webm

@Bonapara @lucasbordeau

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution :)
I've left comments!

@@ -82,7 +82,7 @@ export const LinkFieldInput = ({

return (
<FieldInputOverlay>
<TextInput
<TextArea
Copy link
Member

Choose a reason for hiding this comment

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

let's leave the text input for links for now

}
`;

export const TextArea = ({
Copy link
Member

Choose a reason for hiding this comment

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

I would name it TextAreaInput to be consistent with others
Could you also add storybook story on top of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @charlesBochet, I wanted to check with you regarding the creation of the storybook story for this specific file.

It seems that this component is utilized in the TextFieldInput.tsx file, and this file has already been incorporated into the storybook.

Could you confirm if you would like the storybook story to be crafted exclusively for this particular file?

Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I feel we should cover the UI folder but this can wait a bit, LGTM

@@ -5,7 +5,6 @@ import { ThemeType } from './theme';
export const overlayBackground = (props: { theme: ThemeType }) => css`
backdrop-filter: blur(8px);
background: ${props.theme.background.transparent.secondary};
box-shadow: ${props.theme.boxShadow.strong};
Copy link
Member

Choose a reason for hiding this comment

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

why are we removing this?

border-radius: ${({ theme }) => theme.border.radius.sm};
box-sizing: border-box;
color: ${({ theme }) => theme.font.color.primary};
font-family: inherit;
Copy link
Member

Choose a reason for hiding this comment

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

are all these css properties useful?
font-family inherit?

@@ -56,7 +56,7 @@ export const EmailFieldInput = ({

return (
<FieldInputOverlay>
<TextInput
<TextArea
Copy link
Member

Choose a reason for hiding this comment

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

let's keep email field as TextInput, multi line is not useful there

@jeet1desai
Copy link
Contributor Author

Hello @charlesBochet, All the comment is resolved now, can you please check and let me know if there is anything you want to change.

Thank you.

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@charlesBochet charlesBochet merged commit 74a54da into twentyhq:main Jan 18, 2024
10 checks passed
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.

Replace Text input by Texterarea for field type TEXT
5 participants