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

Support for unit in TextField #1152

Merged
merged 32 commits into from
Mar 16, 2021
Merged

Support for unit in TextField #1152

merged 32 commits into from
Mar 16, 2021

Conversation

wenche
Copy link
Contributor

@wenche wenche commented Mar 9, 2021

Resolve #1111 and #1128

@wenche wenche added the core-react eds-core-react label Mar 9, 2021
@wenche wenche self-assigned this Mar 9, 2021
@wenche
Copy link
Contributor Author

wenche commented Mar 9, 2021

We discussed to maybe use some grid or flex woo that would collapse if no input icon or unit were present. But reusing the Input component will make that hard to achieve, so I'll stick to an approach based on absolute positioning.

@wenche
Copy link
Contributor Author

wenche commented Mar 9, 2021

It's a lot easier to add things in Figma than in code and it would definitely be easier if the unit was to the right of the input 😓

The approach with absolute positioning don't work on unit with unknown width. To add the icon inside the input was easier as icon has a known size and we could add some padding to the text input field to make space for the icon. But with the flexible unit, it doesn't look good. See screenshot below
Skjermbilde 2021-03-09 kl  15 46 44

So I think we'll need to somehow wrap the Input, add unit and/or icon to the right and then create a fake background. This is how Material UI does this as well, using a lot of code to accomplish this. It will be a challenge to get all the variants, focus and so on back on track, and probably a lot of duplicate styling code.

Skjermbilde 2021-03-09 kl  16 25 16
Skjermbilde 2021-03-09 kl  16 25 19
Skjermbilde 2021-03-09 kl  16 25 27

It might actually be worth to consider doing this inside of the Input component itself. But I fear that would be a lot more work because we'll need to adjust the Select as well. And EDS users of the Input component would probably expect a simple Input without a lot of bells and whistles...

@wenche
Copy link
Contributor Author

wenche commented Mar 12, 2021

I'm still finding missing stuffs 🤷‍♀️ The disabled color was not in code.

@wenche
Copy link
Contributor Author

wenche commented Mar 12, 2021

Skjermbilde 2021-03-12 kl  11 03 30
@mimarz This textarea doesn't actually look beautiful, but seriously we cannot keep the version with absolute position in addition to all the new code magic just for this 😬

@mimarz
Copy link
Contributor

mimarz commented Mar 12, 2021

Skjermbilde 2021-03-12 kl 11 03 30
@mimarz This textarea doesn't actually look beautiful, but seriously we cannot keep the version with absolute position in addition to all the new code magic just for this 😬

ugh, I was hoping we could drop absolute positioning all-together, but hmm. One option could be an absolute container with padding equal to the width of that container on the input, but that feels a tad hacky tho.... What are your thoughts? @wenche

@wenche
Copy link
Contributor Author

wenche commented Mar 12, 2021

Skjermbilde 2021-03-12 kl 11 03 30
@mimarz This textarea doesn't actually look beautiful, but seriously we cannot keep the version with absolute position in addition to all the new code magic just for this 😬

ugh, I was hoping we could drop absolute positioning all-together, but hmm. One option could be an absolute container with padding equal to the width of that container on the input, but that feels a tad hacky tho.... What are your thoughts? @wenche

I suggest to just use resize=none I think this icon inside text area is a use case that I don't actually see. For validation we support the icon to the left of the helper text as an option as well

@mimarz
Copy link
Contributor

mimarz commented Mar 12, 2021

Skjermbilde 2021-03-12 kl 11 03 30
@mimarz This textarea doesn't actually look beautiful, but seriously we cannot keep the version with absolute position in addition to all the new code magic just for this 😬

ugh, I was hoping we could drop absolute positioning all-together, but hmm. One option could be an absolute container with padding equal to the width of that container on the input, but that feels a tad hacky tho.... What are your thoughts? @wenche

I suggest to just use resize=none I think this icon inside text area is a use case that I don't actually see. For validation we support the icon to the left of the helper text as an option as well

Thats fair tbh, we should document that people should then use the native rows to control the size of the TextField when multiline is set to true.

@wenche
Copy link
Contributor Author

wenche commented Mar 12, 2021

Skjermbilde 2021-03-12 kl 11 03 30
@mimarz This textarea doesn't actually look beautiful, but seriously we cannot keep the version with absolute position in addition to all the new code magic just for this 😬

ugh, I was hoping we could drop absolute positioning all-together, but hmm. One option could be an absolute container with padding equal to the width of that container on the input, but that feels a tad hacky tho.... What are your thoughts? @wenche

I suggest to just use resize=none I think this icon inside text area is a use case that I don't actually see. For validation we support the icon to the left of the helper text as an option as well

Thats fair tbh, we should document that people should then use the native rows to control the size of the TextField when multiline is set to true.

Yup, I'll add a line or two about that in the storybook

@wenche wenche marked this pull request as ready for review March 12, 2021 15:14
@wenche wenche requested review from mimarz, pomfrida and vnys as code owners March 12, 2021 15:14
Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

Reviewed in pair by @mimarz & @pomfrida.

Lots of good changes here! We like the way you wrote token values in css some places 😍
Mostly suggestions on cleaner code and some new stuff for discussion 😅

We also found a small visual bug when using multilines with scroll-bar and box-shadow as border. Not breaking (and in production). If you know of a fix for it, or else we can live with it I guess.

image

libraries/core-react/src/components/Input/Input.tsx Outdated Show resolved Hide resolved
Comment on lines 28 to 37
box-shadow: ${() =>
isFocused
? `none`
: variant === 'default'
? `inset 0 -1px 0 0 ${
token.border?.type === 'border' && token.border?.color
}`
: `0 0 0 1px ${
token.border?.type === 'border' && token.border?.color
}`};
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this needs to be inside an anonymous function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Struggled a lot to find the easiest syntax for this 🔥 styled-components. Couldn't make it work without the func Might be a user error, but I gave up. 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested, and I couldn't not see any difference without the removed func, so I suggest we remove it to avoid unintended consequences in the future

spacings: { comfortable },
} = tokens

export const textfield: ComponentToken = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend creating a TextFieldToken with a intersection type for which entities are present to help typing. In this case we are missing "unit".

export type TextFieldToken = ComponentToken & {
  entities?: { unit?: ComponentToken }
}

Comment on lines -111 to -122
<RelativeContainer>
{inputIcon ? (
<>
<PaddedInputWrapper {...inputProps} />
<Icon isDisabled={disabled} variant={variant}>
{inputIcon}
</Icon>
</>
) : (
<InputWrapper {...inputProps} />
)}
</RelativeContainer>
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥😍

)
Multiline.parameters = {
docs: {
storyDescription: `With multiline we recommend to use <code>rows</code> in combination with a CSS rule of
Copy link
Contributor

Choose a reason for hiding this comment

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

Should maybe resize be set as default to none internally (with a default number of rows). Created a separate issue for this to be discussed later. #1155

@mimarz
Copy link
Contributor

mimarz commented Mar 15, 2021

Storybook is also complaining on duplicate ids.
image

@wenche
Copy link
Contributor Author

wenche commented Mar 15, 2021

Storybook is also complaining on duplicate ids.
image

Aaarg, forgot to test with the a11y plugin, would have seen it there as well :)

@wenche wenche requested a review from mimarz March 15, 2021 18:54
Copy link
Contributor

@pomfrida pomfrida left a comment

Choose a reason for hiding this comment

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

Just nitpicking, there's also a few label="Multline" typos left in Textfield.stories, just do a search and replace 👍

@wenche wenche requested a review from pomfrida March 16, 2021 10:07
Copy link
Contributor

@pomfrida pomfrida left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

Looks good! I think we should still remove some code as suggested to keep it clean 😅

Comment on lines 28 to 37
box-shadow: ${() =>
isFocused
? `none`
: variant === 'default'
? `inset 0 -1px 0 0 ${
token.border?.type === 'border' && token.border?.color
}`
: `0 0 0 1px ${
token.border?.type === 'border' && token.border?.color
}`};
Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested, and I couldn't not see any difference without the removed func, so I suggest we remove it to avoid unintended consequences in the future

@wenche wenche requested a review from mimarz March 16, 2021 11:17
Copy link
Contributor

@mimarz mimarz left a comment

Choose a reason for hiding this comment

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

awesome

@wenche wenche merged commit f99ec6b into equinor:develop Mar 16, 2021
@wenche wenche deleted the textfield-unit branch March 16, 2021 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-react eds-core-react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit to TextField component
4 participants