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

Issue/167 #180

Merged
merged 12 commits into from
Feb 19, 2020
Merged

Issue/167 #180

merged 12 commits into from
Feb 19, 2020

Conversation

fraincs
Copy link
Collaborator

@fraincs fraincs commented Feb 10, 2020

Issue:
#167 #181 #198 #200

TextArea was not supported in Orbit.

What I did

Created a component TextArea which is a HOC of Semantic UI TextArea component. This component supports success, transparent, error, autofocusdelay

How to test

  • Is this testable with Jest or Chromatic screenshots?
    Both.

  • Does this need an update to the documentation?
    Yes, documentation has been created.

@netlify
Copy link

netlify bot commented Feb 10, 2020

Deploy preview for sg-storybook ready!

Built with commit fc669bb

https://deploy-preview-180--sg-storybook.netlify.com

@netlify
Copy link

netlify bot commented Feb 10, 2020

Deploy preview for sg-orbit ready!

Built with commit fc669bb

https://deploy-preview-180--sg-orbit.netlify.com


const textAreaRef = useRef();

useImperativeHandle(forwardedRef, () => getTextAreaElement(textAreaRef));
Copy link
Member

Choose a reason for hiding this comment

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

you don't have to use useImperativeHandle in this case, simply pass forwardRef to Ref instead of creating a proxy ref.

<Ref innerRef={forwardRef} ...

@patricklafrance
Copy link
Member

Should we support size?

We already have rows but it's not the same. size would also alter font-size. If you support size do we want to support rows ?

@fraincs
Copy link
Collaborator Author

fraincs commented Feb 11, 2020

We already have rows but it's not the same. size would also alter font-size. If you support size do we want to support rows ?

rows is a default attribute of textarea

@fraincs
Copy link
Collaborator Author

fraincs commented Feb 11, 2020

Should we support size?

This is something to discuss with designers, I think it doesn't make sense height wise, but font sizes should match the input's font size, a different line-height might be a good idea too.

@fraincs
Copy link
Collaborator Author

fraincs commented Feb 11, 2020

Should we support size?

We should follow the same padding, font-size, as same size input, I will adjust before closing this PR.

@fraincs
Copy link
Collaborator Author

fraincs commented Feb 11, 2020

A Jest test is failing, this seems to be the same issue as this: testing-library/jest-dom#53

@patricklafrance
Copy link
Member

patricklafrance commented Feb 11, 2020

A Jest test is failing, this seems to be the same issue as this: testing-library/jest-dom#53

Yes, we already use the user-event library to mitigates those problems.

I wonder if this is the same problem you're experiencing though since the component you are testing is explicitly focusing on a dom element, this is not the same as triggering a click event and expecting the target element to have focus.

<Preview>
<Story name="size">
<div className="flex flex-row items-end items-space-between">
<div className="flex-auto">
Copy link
Member

Choose a reason for hiding this comment

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

Do you need flex-auto here?

In fact, I added it for the date picker but I don't understand why we need flex-auto.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See flex auto explanation on MDN : The item is sized according to its width and height properties, but grows to absorb any extra free space in the flex container, this is the equivalent of "flex: 1 1 auto"

soit flex grow 1 shrink1 basis auto

@patricklafrance
Copy link
Member

Like an input, IMO a textarea shouldn't be fluid by default?

It should support a fluid prop.

@patricklafrance patricklafrance merged commit 1a6407e into master Feb 19, 2020
@patricklafrance patricklafrance deleted the issue/167 branch February 19, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants