carbon-design-system / carbon-react-native

The React Native implementation of the Carbon Design System
https://carbon-design-system.github.io/carbon-react-native/
Apache License 2.0
30 stars 8 forks source link

[Design review] Text input (BaseTextInputs) #27

Closed 8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I closed 2 years ago

8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago
image
8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

Vince when tacking this component did someting like this about the borders 1 vs 2px and paddings.

inputContainer: (theme: Theme, status: Status): ViewStyle => {
    let borderColor = theme.ui04;

    if (status === 'active') {
      borderColor = theme.focus;
    }

    if (status === 'error') {
      borderColor = theme.support01;
    }
    return {
      flexDirection: 'row',
      justifyContent: 'center',
      alignItems: 'center',
      backgroundColor: theme.field01,
      borderColor: borderColor,
      borderWidth: status ? 2 : 0,
      borderBottomWidth: status ? 2 : 1,
      paddingLeft: status ? 16 : 18,
      paddingTop: status ? 12 : 14,
      paddingBottom: status ? 12 : 13,
      paddingRight: status ? 14 : 16,
    };
  },
  caption: (theme: Theme, status: Status): TextStyle => ({
    color: status === 'error' ? theme.support01 : theme.text02,
    marginTop: 8,
  }),
  input: (theme: Theme) => ({
    color: theme.text01,
    fontSize: 16,
    flexGrow: 1,
    flexShrink: 1,
  }),
};
WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Rules from the Internationalization team we use does not allow this I would truncate the label and wouldn't allow for multi-line.

Apps should try to avoid labels that are that long. But for the test app I include worst case for all things to just make sure stuff does not break.

I think truncating should be avoided on mobile since no real way to read it all without hover or other interactions to peak into it.

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

I got the 2px from the Carbon 11 code for this. But can change to 1px Screen Shot 2022-08-12 at 8 55 48 AM

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

label-02 had a bug. Sorry. I will review all the typography and make sure they match https://carbondesignsystem.com/guidelines/typography/type-sets

8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

I got the 2px from the Carbon 11 code for this. But can change to 1px Screen Shot 2022-08-12 at 8 55 48 AM

Indeed errror and focus should be 2px, but it seems like currently it is 1px only. See the screenshot.

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

I got the 2px from the Carbon 11 code for this. But can change to 1px Screen Shot 2022-08-12 at 8 55 48 AM

Indeed errror and focus should be 2px, but it seems like currently it is 1px only. See the screenshot.

Sorry T_T (should drink more coffee its still too early) I totally read that backwards above 😢

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Fixed. See screenshots for each color theme and component (all text like component/rules come from central place so applies to all).

Screen Shot 2022-08-12 at 9 41 54 AM Screen Shot 2022-08-12 at 9 42 03 AM Screen Shot 2022-08-12 at 9 43 48 AM Screen Shot 2022-08-12 at 9 44 05 AM Screen Shot 2022-08-12 at 9 41 28 AM Screen Shot 2022-08-12 at 9 35 36 AM Screen Shot 2022-08-12 at 9 35 08 AM Screen Shot 2022-08-12 at 9 34 56 AM

8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

Most changes look great, than you.

One thing I noticed – the position of the error icon. See the attached images for a guidance. In most cases the plus, minus, show/hide icons are really a button component with 48px tap area (in the future we can add tapped state etc). This 48px includes/embraces/overlaps the border around the input (but border is "above" - matters only when we have a tapped state with different background).

You can see that in password and number input error icon is directly next to the buttons. For the text input and text area it is inside the 48px box for better alignment.

image image image image
8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

Here I think it is ok to truncate :) iOS has an ability to auto add "…" character, hope this is possible in RN too.

Image from iOS (1)

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Here I think it is ok to truncate :) iOS has an ability to auto add "…" character, hope this is possible in RN too.

Image from iOS (1)

That is weird. That should use the react native TextInput. Will see why it’s breaking out. It shouldn’t…. Should follow OS rules.

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

For the wrapping in TextInput when not multiline seems there is an open bug. But also a workaround. Will add workaround until bug is fixed. https://github.com/facebook/react-native/issues/29068

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Changes on the error button (background color added to show overlay)

Screen Shot 2022-08-12 at 3 33 57 PM Screen Shot 2022-08-12 at 3 34 46 PM

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

I added an extra prop so consumers of the app can choose to break labels with ellipsis. By default it will wrap.

  /** Label break mode */
  labelBreakMode?: TextBreakModes;
8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

Most changes look great, than you.

One thing I noticed – the position of the error icon. See the attached images for a guidance. In most cases the plus, minus, show/hide icons are really a button component with 48px tap area (in the future we can add tapped state etc). This 48px includes/embraces/overlaps the border around the input (but border is "above" - matters only when we have a tapped state with different background).

You can see that in password and number input error icon is directly next to the buttons. For the text input and text area it is inside the 48px box for better alignment.

image image image image

Seems like the Password input and Number input don't have the updates described above - please take a look at the picture and you will see the error icon is directly attached to the buttons on its right side, a not contained in 48x48 area.

8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

Another thing I noticed – it seems like all the input fields do not change back to regular state when no longer in focus, so you can have 3 items on the screen and all look like they are in focus, but only one (or none) really is.

image

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

For the icon it seems to be a bit further away. It gives 96 space for the number and 48 for the password.

Is this not right?

DDE30BBE-E754-4084-B33F-9874D9C99812 2EAAE97B-AE1C-451F-B01A-AE6A3562B070

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

So after some very confusing research :/ the bug in React Native that lets text escape (https://github.com/facebook/react-native/issues/29068) my workaround which keeps text from escaping causes border to get stuck (it doesn't change colors for some reason; even though the component gets proper color)...

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Border color getting stuck is fixed.

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Confirm if you don't see the proper sizing for the error icon (it should have been in version 1.0.0 (5)). you can see your version on the Home Screen (when you first open the app or the info popup).

8OYx4JT4MsadkJ5CPwN52Fdm7fbRLSNYd0h4y5I commented 2 years ago

Confirm if you don't see the proper sizing for the error icon (it should have been in version 1.0.0 (5)). you can see your version on the Home Screen (when you first open the app or the info popup).

I'm on 1.0.0 (6) and for the Password and Number Input it still seems to be withing 48x48 container. For this two cases it shouldn't be contained, as container adds additional side paddings. For this two cases the icon should be not contained, so it directly "touches" the eye or plus/minus buttons (these are 48x48 of course). See the image I posted above and compare these two components vs regular input and text area and it will be clear.

Thanks for the focus state fix, it is working great now.

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Ohh. For the icon you DO NOT want it to be 48px wide (same as buttons). You want it to press against the action? The images you posted were what it should be not the bug.

If so that goes against Carbon for web right?

1BC36B26-53F1-4759-A057-FC8759BA39F4

WYXEhlEtzb2Gfe6HgrQLAf6MeMEatE77yYzHHIH commented 2 years ago

Removed padding on left/right for error for NUMBER/PASSWORD

Screen Shot 2022-08-15 at 10 50 53 AM Screen Shot 2022-08-15 at 10 50 30 AM Screen Shot 2022-08-15 at 10 50 16 AM