[FX-4599] Migrate OutlineInput #4363

ruslan-sed · 2024-06-13T15:14:16Z

Description

Migrate OutlinedInput to TailwindCSS.

How to test

Temploy
FIXME: Add the steps describing how to verify your changes

Note

Visual tests for Select with a focused state after click are failing. There is no box shadow and correct border. But in browser, they are. The issue occurs only in Cypress. I and @TomasSlama could not figure out for now the reason. Since it works in browser, we agreed, that we can merge it now and fix it in a follow-up ticket.

Development checks

Add changeset according to guidelines (if needed)
Read CONTRIBUTING.md and Component API principles
Make sure that additions and changes on the design follow Toptal's BASE design, and it's been already discussed with designers at #-base-core
Annotate all props in component with documentation
Create examples for component
Ensure that deployed demo has expected results and good examples
Ensure the changed/created components have not caused accessibility issues. How to use accessibility plugin in storybook.
Self reviewed
Covered with tests (visual tests included)

Breaking change

~~codemod is created and showcased in the changeset~~
~~test alpha package of Picasso in StaffPortal~~

All development checks should be done and set checked to pass the
GitHub Bot: TODOLess action

PR commands

List of available commands:

@toptal-bot run package:alpha-release - Release alpha version
@toptal-anvil ping reviewers - Ping FX team for review

PR Review Guidelines

When to approve? ✅

You are OK with merging this PR and

You have no extra requests.
You have optional requests.
1. Add nit: to your comment. (ex. nit: I'd rename this variable from makeCircle to getCircle)

When to request changes? ❌

You are not OK with merging this PR because

Something is broken after the changes.
Acceptance criteria is not reached.
Code is dirty.

When to comment (neither ✅ nor ❌)

You want your comments to be addressed before merging this PR in cases like:

There are leftovers like unnecessary logs, comments, etc.
You have an opinionated comment regarding the code that requires a discussion.
You have questions.

How to handle the comments?

An owner of a comment is the only one who can resolve it.
An owner of a comment must resolve it when it's addressed.
A PR owner must reply with ✅ when a comment is addressed.

changeset-bot · 2024-06-13T15:14:20Z

🦋 Changeset detected

Latest commit: 5f0ced2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages

Name	Type
@toptal/picasso-outlined-input	Major
@toptal/picasso-forms	Patch
@toptal/picasso-rich-text-editor	Patch
@toptal/picasso	Patch
@toptal/picasso-autocomplete	Patch
@toptal/picasso-avatar-upload	Patch
@toptal/picasso-date-picker	Patch
@toptal/picasso-input	Patch
@toptal/picasso-number-input	Patch
@toptal/picasso-password-input	Patch
@toptal/picasso-select	Patch
@toptal/picasso-tagselector	Patch
@toptal/picasso-timepicker	Patch
@toptal/picasso-page	Patch
@toptal/picasso-query-builder	Patch
@toptal/picasso-date-select	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ruslan-sed · 2024-06-21T13:19:07Z

@toptal-anvil ping reviewers

ruslan-sed · 2024-06-24T08:23:30Z

@toptal-anvil ping reviewers

TomasSlama · 2024-06-24T08:26:45Z

packages/base/OutlinedInput/package.json

@@ -28,25 +28,22 @@
    "@toptal/picasso-input-adornment": "1.0.7",
    "@toptal/picasso-shared": "15.0.0",
    "@toptal/picasso-utils": "1.0.3",
+    "@toptal/picasso-tailwind-merge": "1.0.1",


let's move @toptal/picasso-tailwind-merge to peer dependencies and dev dependencies

packages/base/OutlinedInput/src/OutlinedInput/OutlinedInput.tsx

packages/base/OutlinedInput/src/OutlinedInput/utils.ts

TomasSlama · 2024-06-24T08:43:48Z

packages/base/OutlinedInput/src/OutlinedInput/utils.ts

+    disabled && 'text-gray-600 [&::-webkit-text-fill-color]:text-gray-600',
+    // On Safari the text gets a bit lighter as if it had some transparency applied to it
+    // We need this webkit-specific property to achieve the exact font color
+    disabled && '[&::-webkit-text-fill-color]:text-gray-600',
+    // It's a special case for TimePicker
+    type === 'time' && inputProps?.disabled && 'text-black',
+    isDark &&
+      'text-white [&::placeholder]:text-white [&::placeholder]:opacity-[0.64]',


So if we have disabled dark input, it should still have white text color? 🤔

No, because styles for disabled goes after this line, so twMerge will remove text-white.

Sorry. My previous answer was incorrect. Correct answer is - yes. And It is so in master branch.

TomasSlama · 2024-06-24T08:47:51Z

packages/base/OutlinedInput/src/OutlinedInput/utils.ts

+  )
+
+// eslint-disable-next-line complexity
+export const getInputClassName = ({


I'm not sure I like these big functions that deal with everything 🤔 Imagine you would have to test this function, it would be very painful. I prefer smaller functions with a single responsibility and with few inputs

Since we already have visual tests I was thinking about this function as something we will need to test. Its purpose here is just to decrease the complexity of the component. Sure, we can split it into smaller functions, but will it give us any benefit, considering that the effect of this function is already covered with more high-level tests?

The main reason why I think it is better to have one function is that all classes are in the same place. It is easier to work with them, and find issues, like conflicting classes that override each other and mess up styling.

Visual tests are second-class tests, IMO. Unit tests and integration tests are much more important from a development perspective; visual tests are only there to test the aesthetic parts, not behavior.

Complexity is not a problem only about testability, but it is also a problem with readability. We need to think of how other developers will read this code in future, walking through all of these conditions, sometimes you (the original writer) will be the one to re-unveil its secrets.

I agree with Tomas, just moving the complexity from a function to another doesn't solve the problem, it might make the main component function aesthetically "pleasing" but we still need to walk through the complexity if we want to understand how the styling works, now with some extra 20 lines for the function definition. I believe this function is clear, but perhaps we should remove the extra lines for restructuring and leave it as props: Props, and just change the references to props.prop. Can you also add a unit test for it? Tests are also good documentation complements, as they make clear what are our intents in an English text, having it as a separate function is a great step on making the code testable

I believe that adding tests here to be a good opportunity to improve on this component, which will become one of the most complex we have

Tests should be easy and fun, you can just import it and check for all conditionals, example:

import { getInputClassName } from './utils' describe('getInputClassName', () => { describe('when disabled', () => { it('styles webkit fill color', () => { const result = getInputClassName({ disabled }) expect(result).toContain('[&::-webkit-text-fill-color]:text-gray-600') }) // ... }) })

I see now the benefit of testing these functions. But I'm not sure about props.prop . I see, that we don't use this approach in our components and util functions. It happens from time to time, mostly in tests. So I prefer to leave destructuring. Are you ok with that @augustobmoura, @TomasSlama?

TomasSlama · 2024-06-24T09:08:39Z

packages/base/OutlinedInput/src/OutlinedInput/utils.ts

+    width === 'shrink' && 'w-auto',
+    width === 'auto' && 'w-[18.75rem]',


even though this looks weird, it is correct, I double checked with master ;-)

augustobmoura

Checked the code, but we still have a few differences on Happo that need to be addressed. I believe adding unit tests here will also be a huge improvement for the future, this is a major component on our stack and having it well tested is very important

augustobmoura · 2024-06-25T13:53:16Z

packages/base/OutlinedInput/src/OutlinedInput/OutlinedInput.tsx

+  return multiline ? (
+    <Input
+      {...sharedProps}
+      multiline={true}
+      rows={getRows(rows)}
+      maxRows={getRows(rowsMax)}
    />
+  ) : (
+    <Input {...sharedProps} />


If the component is the same and we only change multiline, rows, maxRows, perhaps we should only have them conditionally added, while also keeping the props inlined with the element

Suggested change

return multiline ? (

<Input

{...sharedProps}

multiline={true}

rows={getRows(rows)}

maxRows={getRows(rowsMax)}

/>

) : (

<Input {...sharedProps} />

return (

<Input

className='...'

style={{}}

// ...

multiline={multiline ? true : undefined}

rows={multiline ? getRows(rows) : undefined}

maxRows={multiline ? getRows(rowsMax) : undefined}

/>

)

It will not work like this due to Input props type. It is a union of two different types. And for the one where multiline is true, rows and maxRows should not be undefined. And due to that it should be splited to two different returns. Otherwise, TS will throw an error.

Got it. We still can have only the multiline props on an object while the remaining is inlined without the ternary expression, wdyt?
Having most of props on a object instead of inlined on the JSX element is a bit unstylistic and we lose

Suggested change

return multiline ? (

<Input

{...sharedProps}

multiline={true}

rows={getRows(rows)}

maxRows={getRows(rowsMax)}

/>

) : (

<Input {...sharedProps} />

const multilineProps = multiline

? ({

multiline: true,

rows: getRows(rows),

maxRows: getRows(rowsMax),

} as const)

: {}

return (

<Input

className='...'

style={{}}

// ...

{...multilineProps}

/>

This is nice 🚀

augustobmoura · 2024-06-25T14:06:23Z

packages/base/OutlinedInput/src/OutlinedInput/utils.ts

+  )
+
+// eslint-disable-next-line complexity
+export const getInputClassName = ({


Visual tests are second-class tests, IMO. Unit tests and integration tests are much more important from a development perspective; visual tests are only there to test the aesthetic parts, not behavior.

Complexity is not a problem only about testability, but it is also a problem with readability. We need to think of how other developers will read this code in future, walking through all of these conditions, sometimes you (the original writer) will be the one to re-unveil its secrets.

I agree with Tomas, just moving the complexity from a function to another doesn't solve the problem, it might make the main component function aesthetically "pleasing" but we still need to walk through the complexity if we want to understand how the styling works, now with some extra 20 lines for the function definition. I believe this function is clear, but perhaps we should remove the extra lines for restructuring and leave it as props: Props, and just change the references to props.prop. Can you also add a unit test for it? Tests are also good documentation complements, as they make clear what are our intents in an English text, having it as a separate function is a great step on making the code testable

I believe that adding tests here to be a good opportunity to improve on this component, which will become one of the most complex we have

Tests should be easy and fun, you can just import it and check for all conditionals, example:

import { getInputClassName } from './utils' describe('getInputClassName', () => { describe('when disabled', () => { it('styles webkit fill color', () => { const result = getInputClassName({ disabled }) expect(result).toContain('[&::-webkit-text-fill-color]:text-gray-600') }) // ... }) })

ruslan-sed · 2024-06-25T15:07:03Z

@augustobmoura there is a note about those failing tests in the PR description

augustobmoura

Left a small comment at #4363 (comment), please resolve it before merging.

GJ on the PR! 👍

TomasSlama · 2024-06-27T10:46:07Z

packages/base/OutlinedInput/src/OutlinedInput/OutlinedInput.tsx


-import styles from './styles'
+import { getInputClassName, getRootClassName, getRows } from './styles/styles'


just for consistency, we don't have styles folder in any component, we can just have OutlinedInput/styles.ts

TomasSlama · 2024-06-27T10:48:40Z

packages/base/OutlinedInput/src/OutlinedInput/OutlinedInput.tsx

+          className: getRootClassName({
+            size,
+            width,
+            type,
+            layout,
+            isDark,
+            multiline,
+            highlight,
+            disabled,
+            className,
+            classes,
+            isError,
+          }),


Suggested change

className: getRootClassName({

size,

width,

type,

layout,

isDark,

multiline,

highlight,

disabled,

className,

classes,

isError,

}),

className: twMerge(

getRootClassName({

size,

width,

type,

layout,

isDark,

multiline,

highlight,

disabled,

isError,

}),

className,

classes.root

),

TomasSlama · 2024-06-27T10:53:45Z

packages/base/OutlinedInput/src/OutlinedInput/styles/styles.ts

+      'text-gray-600',
+      // On Safari the text gets a bit lighter as if it had some transparency applied to it
+      // We need this webkit-specific property to achieve the exact font color
+      disabled ? '[&::-webkit-text-fill-color]:text-gray-600' : '',


this condition will be always true as it is already in if (disabled)

TomasSlama · 2024-06-27T10:55:11Z

packages/base/OutlinedInput/src/OutlinedInput/styles/styles.ts

+      // On Safari the text gets a bit lighter as if it had some transparency applied to it
+      // We need this webkit-specific property to achieve the exact font color
+      disabled ? '[&::-webkit-text-fill-color]:text-gray-600' : '',
+    ].join(' ')


Suggested change

].join(' ')

]

twJoin and twMerge accepts arrays, so we don't need to join these. It can even work with nested arrays

ruslan-sed · 2024-06-27T15:37:23Z

@TomasSlama

After refactoring visual tests provides the expected result. What is left is to add tests for style utils.

I have a concern about styles.ts file. It became too big. I know that in other components we usually have only one styles.ts, but in this case for readability purposes maybe we should separate the content of our styles.ts into two different files for root and input functions. In this case files will be smaller and simpler to read and work with them.

And in this case, it also can make sense to have them in a separate folder, where we also can have tests for them.

ruslan-sed force-pushed the fx-4599-migrate-outlineinput branch 12 times, most recently from 85ffac2 to 9a770cc Compare June 19, 2024 15:08

ruslan-sed force-pushed the fx-4599-migrate-outlineinput branch 2 times, most recently from 0d7c2f6 to d49ccd6 Compare June 21, 2024 12:58

ruslan-sed marked this pull request as ready for review June 21, 2024 13:00

ruslan-sed requested a review from a team as a code owner June 21, 2024 13:00

TomasSlama reviewed Jun 24, 2024

View reviewed changes

augustobmoura reviewed Jun 25, 2024

View reviewed changes

ruslan-sed added 3 commits June 26, 2024 16:34

[FX-4599] Migrate OutlinedInput to TailwindCSS

50c8482

Address Tomas's comments

a8d3f9f

Add tests for utils

d8020fb

ruslan-sed force-pushed the fx-4599-migrate-outlineinput branch from 0998017 to d8020fb Compare June 26, 2024 13:34

augustobmoura approved these changes Jun 27, 2024

View reviewed changes

Refactor to return without ternary operator

1d4d102

TomasSlama reviewed Jun 27, 2024

View reviewed changes

ruslan-sed force-pushed the fx-4599-migrate-outlineinput branch from d68ae19 to 9d272d9 Compare June 27, 2024 14:29

ruslan-sed force-pushed the fx-4599-migrate-outlineinput branch from 174b904 to 86bf502 Compare June 27, 2024 15:22

Refactor style utils

08b0403

ruslan-sed force-pushed the fx-4599-migrate-outlineinput branch from 86bf502 to 08b0403 Compare June 27, 2024 15:25

Refactor styles

bd9e39a

TomasSlama approved these changes Jun 28, 2024

View reviewed changes

TomasSlama added 2 commits June 28, 2024 15:55

Merge branch 'feature/tailwind-w21' into fx-4599-migrate-outlineinput

25f2f48

Update snapshots

5f0ced2

TomasSlama merged commit 49984f1 into feature/tailwind-w21 Jun 28, 2024
7 checks passed

TomasSlama deleted the fx-4599-migrate-outlineinput branch June 28, 2024 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FX-4599] Migrate OutlineInput #4363

[FX-4599] Migrate OutlineInput #4363

ruslan-sed commented Jun 13, 2024 •

edited

Loading

changeset-bot bot commented Jun 13, 2024 •

edited

Loading

ruslan-sed commented Jun 21, 2024

ruslan-sed commented Jun 24, 2024

TomasSlama Jun 24, 2024

TomasSlama Jun 24, 2024

ruslan-sed Jun 25, 2024

ruslan-sed Jun 26, 2024

TomasSlama Jun 24, 2024

ruslan-sed Jun 25, 2024

ruslan-sed Jun 25, 2024 •

edited

Loading

augustobmoura Jun 25, 2024

ruslan-sed Jun 25, 2024

TomasSlama Jun 24, 2024

augustobmoura left a comment

augustobmoura Jun 25, 2024

ruslan-sed Jun 25, 2024

augustobmoura Jun 27, 2024 •

edited

Loading

ruslan-sed Jun 27, 2024

augustobmoura Jun 25, 2024

ruslan-sed commented Jun 25, 2024

augustobmoura left a comment

TomasSlama Jun 27, 2024

TomasSlama Jun 27, 2024

TomasSlama Jun 27, 2024

TomasSlama Jun 27, 2024

ruslan-sed commented Jun 27, 2024 •

edited

Loading

		width === 'shrink' && 'w-auto',
		width === 'auto' && 'w-[18.75rem]',

-  return multiline ? (
-    <Input
-      {...sharedProps}
-      multiline={true}
-      rows={getRows(rows)}
-      maxRows={getRows(rowsMax)}
-    />
-  ) : (
-    <Input {...sharedProps} />
+  const multilineProps = multiline
+    ? ({
+        multiline: true,
+        rows: getRows(rows),
+        maxRows: getRows(rowsMax),
+      } as const)
+    : {}
+  return (
+    <Input
+      className='...'
+      style={{}}
+      // ...
+      {...multilineProps}
+    />


		import styles from './styles'
		import { getInputClassName, getRootClassName, getRows } from './styles/styles'

[FX-4599] Migrate OutlineInput #4363

[FX-4599] Migrate OutlineInput #4363

Conversation

ruslan-sed commented Jun 13, 2024 • edited Loading

Description

How to test

Development checks

When to approve? ✅

When to request changes? ❌

When to comment (neither ✅ nor ❌)

How to handle the comments?

changeset-bot bot commented Jun 13, 2024 • edited Loading

🦋 Changeset detected

ruslan-sed commented Jun 21, 2024

ruslan-sed commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruslan-sed Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustobmoura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustobmoura Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruslan-sed commented Jun 25, 2024

augustobmoura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruslan-sed commented Jun 27, 2024 • edited Loading

ruslan-sed commented Jun 13, 2024 •

edited

Loading

changeset-bot bot commented Jun 13, 2024 •

edited

Loading

ruslan-sed Jun 25, 2024 •

edited

Loading

augustobmoura Jun 27, 2024 •

edited

Loading

ruslan-sed commented Jun 27, 2024 •

edited

Loading