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.

Sign up for GitHub

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

Jump to bottom

#342 Support type prop to set the type of input, adjusted styles to fit with designs, added Icon functionality #369

Merged

marcinsawicki merged 15 commits into v1 from feature/342

Nov 25, 2022

Contributor

marcinsawicki commented Jul 28, 2022 •

edited

Loading

Resolves: #342

Description

The Input component is missing some functionalities after migration from the old implementation. Also, we found that the component has incorrect or missing styles.
What's changed:

Added support for native type prop to enable switching between text and password mode, where text is the default
In password mode, added an icon to switch between password visibility
Added icon prop for passing the component, which will be displayed as decoration in input
Refactored code and styles to fit with the designs
Changes size prop to kind to set the input size, because size is reserved in native props and has a different desitnation

Storybook

Storybook

Checklist

Obligatory:

Optional:

Accessibility cases (keyboard control, correct HTML markup, etc.)

Szymon Graczyk and others added 2 commits

July 27, 2022 10:54


          #339 Apply missing tokens to Input and TagInput components

0b22563


          input styles update, added icon functionality, added kinds with password

3ee38d1

marcinsawicki added the feature label

marcinsawicki linked an issue

that may be closed by this pull request

[Input] Add missing functionalities and enable possibility the type of input #342

Closed

marcinsawicki marked this pull request as draft

July 28, 2022 08:09


          unit tests

26520a3

marcinsawicki marked this pull request as ready for review

July 28, 2022 10:24


          Merge branch 'v1' into feature/342

9ced230

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.stories.tsx Outdated

    
            @@ -1,15 +1,20 @@
          
              import * as React from 'react';

              import { ComponentMeta, Story } from '@storybook/react';

              import { StoryDescriptor } from '../../stories/components/StoryDescriptor';

              import * as MaterialIcons from '@livechat/design-system-icons/react/material';

sgraczyk Jul 28, 2022

You can remove the line break from 3

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.stories.tsx Outdated

+              export const Kinds = (): JSX.Element => (
+                <>
+                  <StoryDescriptor title="Text">
+                    <Input placeholder={placeholderText} />

sgraczyk Jul 28, 2022

While defining specific variants, we should not rely on default values as it's intended to include prop value for docs


          changes after review

c1f0cba

marcinsawicki requested review from sgraczyk and a team

July 28, 2022 12:21

kstvsko self-requested a review

August 1, 2022 09:16

kstvsko approved these changes

View reviewed changes

kstvsko left a comment

As for now, it is compliant with the designs. The only thing I would rethink is if we want to have a hover state on the clickable icons like the one for showing/hiding password. I will take care of the topic and bring a new issue if needed.

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.stories.tsx

+              );
+              export const WithIcon = (): JSX.Element => (
+                <>

sgraczyk Aug 18, 2022

React.Fragment is not necessary here

sgraczyk Nov 14, 2022

Bump ☝🏼

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.tsx Outdated

               export interface InputProps extends React.HTMLAttributes<HTMLInputElement> {
                 size?: InputSize | undefined;
                 error?: boolean | undefined;
                 disabled?: boolean | undefined;
+                kind?: 'text' | 'password';

sgraczyk Aug 18, 2022

To be honest, I'm not sure if it's a good approach. We already extend React.HTMLAttributes<HTMLInputElement> which has a prop called type And it should handle password type -> https://www.w3schools.com/html/html_form_input_types.asp

Contributor

MateuszRomek Aug 25, 2022

☝️

kstvsko commented Sep 6, 2022

@sgraczyk @MichalPaszowski @marcinsawicki Any idea how the disabled input with icon looks? I had to fix that issue in Figma and I also wanted to take care of it here, but I cannot see the disabled input with the icon on Storybook.


          changes after review

6c98109

marcinsawicki changed the title ~~#342 Added kinds to set the type of input, adjusted styles to fit with designs, added Icon functionality~~ #342 Support type prop to set the type of input, adjusted styles to fit with designs, added Icon functionality

MichalPaszowski requested changes

View reviewed changes

packages/react-components/src/components/Input/Input.tsx Outdated

-                size?: InputSize | undefined;
+              export interface InputProps
+                extends React.InputHTMLAttributes<HTMLInputElement> {
+                kind?: InputSize | undefined;

Contributor

MichalPaszowski Oct 14, 2022

why do you need to | undefined in this and the next three lines


          changes after review

49bac8f

marcinsawicki requested a review from MichalPaszowski

October 28, 2022 05:40


          change the prop name

563aab9

marcinsawicki requested review from sgraczyk and MateuszRomek

November 14, 2022 12:46

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.spec.tsx Show resolved Hide resolved

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.tsx Outdated

-                disabled?: boolean | undefined;
+              export interface InputProps
+                extends React.InputHTMLAttributes<HTMLInputElement> {
+                wrapperSize?: InputSize;

sgraczyk Nov 14, 2022

Maybe inputSize. I'm not sure if wrapper is easy to understand.

Contributor

MateuszRomek Nov 15, 2022

I definitely agree with that. inputSize feels much better

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.tsx Outdated

+                          kind="plain"
+                          icon={
+                            <Icon
+                              customColor={

sgraczyk Nov 14, 2022

I would move these conditions to constants

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Input/Input.tsx Outdated

+                                  : 'var(--content-disabled)'
+                              }
+                              source={
+                                !isPasswordVisible ? VisibilityOnIcon : VisibilityOffIcon

sgraczyk Nov 14, 2022

As above, the target is to make JSX as simple as possible

sgraczyk approved these changes

View reviewed changes

MateuszRomek approved these changes

View reviewed changes

packages/react-components/src/components/Input/Input.tsx Outdated

-                disabled?: boolean | undefined;
+              export interface InputProps
+                extends React.InputHTMLAttributes<HTMLInputElement> {
+                wrapperSize?: InputSize;

Contributor

MateuszRomek Nov 15, 2022

I definitely agree with that. inputSize feels much better

packages/react-components/src/components/Input/Input.tsx Outdated


		type InputSize = 'xsmall' \| 'small' \| 'medium' \| 'large';
		type InputSize = 'compact' \| 'medium' \| 'large';

Contributor

MateuszRomek Nov 15, 2022

For me it could be a generic size type lifted up in folders structure as we are slowly starting to repeat ourselves

marcinsawicki added 3 commits

November 15, 2022 11:53


          changes after review

137281e


          export new Size type

c3271d6


          merge with v1 and resolved conflicts

sgraczyk reviewed

View reviewed changes

packages/react-components/src/components/Popover/Popover.module.scss Outdated

                 z-index: $stacking-context-level-popover;
+                border-radius: 4px;
+                box-shadow: 0 1px 10px 0 rgb(66 77 87 / 30%);

sgraczyk Nov 15, 2022

How about a shadow color from tokens here?

sgraczyk approved these changes

View reviewed changes

marcinsawicki and others added 2 commits

November 16, 2022 10:04


          small change

76106b1


          Merge branch 'v1' into feature/342

33cdf60

Contributor

jpyzio123 commented Nov 18, 2022

Reversed eye icons in type:password. By default the icon should be crossed out indicating the password is hidden

Contributor

jpyzio123 commented Nov 18, 2022 •

edited

Loading

mission icon options to put icon to the right side or two icons on both sides as in designs

marcinsawicki added 2 commits

November 25, 2022 11:14


          changes after tests, added possibility to set the place of the icon

b12f5fc


          Merge branch 'feature/342' of github.com:livechat/design-system into …

e97ea6d

…feature/342

jpyzio123 approved these changes

View reviewed changes

marcinsawicki merged commit 48ecd8a into v1

marcinsawicki deleted the feature/342 branch

November 25, 2022 11:36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kstvsko kstvsko approved these changes

MateuszRomek MateuszRomek approved these changes

jpyzio123 jpyzio123 approved these changes

sgraczyk sgraczyk approved these changes

MichalPaszowski Awaiting requested review from MichalPaszowski

Labels

feature