refactor: migrate Modal to TailwindCSS #3800

TomasSlama · 2023-08-11T17:31:29Z

FX-4034

Description

This PR is the POC of migrating a complex component from MUI4 to TailwindCSS. We chose Modal.

This PR involves:

Install TailwindCSS and make it work in the Storybook
Apply BASE tokens to the TailwindCSS config
Migrate the Modal component

How to test

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

Screenshots

Before.	After.
Insert screenshots or screen recordings	Insert screenshots or screen recordings

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 · 2023-08-11T17:31:32Z

⚠️ No Changeset found

Latest commit: e23089f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

TomasSlama · 2023-08-11T17:32:25Z

package.json

@@ -63,6 +63,7 @@
    "@changesets/get-github-info": "^0.5.1",
    "@cypress/skip-test": "^2.6.1",
    "@storybook/addon-a11y": "^6.5.15",
+    "@storybook/addon-styling": "0.3",


we need to stick to the 0.x version till we update to storybook 7

TomasSlama · 2023-08-11T17:35:05Z

packages/picasso/src/Modal/Modal.tsx

@@ -216,43 +210,50 @@ export const Modal = forwardRef<HTMLElement, Props>(function Modal(props, ref) {
    [disableBackdropClick, onBackdropClick, onClose]
  )

+  useBodyScrollLock(open)


even though Modal from mui is supporting body lock by default, it does not work when we change the container from the body to the Picasso root. It appears to be working even if we change it back to body, but I didn't want to break anything unexpectedly, so keeping the same behavior as on master

TomasSlama · 2023-08-11T17:35:58Z

packages/picasso/src/Modal/Modal.tsx

@@ -216,43 +210,50 @@ export const Modal = forwardRef<HTMLElement, Props>(function Modal(props, ref) {
    [disableBackdropClick, onBackdropClick, onClose]
  )

+  useBodyScrollLock(open)
+
+  const duration = transitionProps?.timeout || transitionDuration


I double checked in MUI implementation that transitionProps are overriding transitionDuration

TomasSlama · 2023-08-11T17:39:15Z

packages/picasso/src/Modal/__snapshots__/test.tsx.snap

    />
    <div
-      data-test="sentinelStart"
+      data-testid="sentinelStart"
      tabindex="0"
    />
    <div


I decided to remove MuiDialog-container as it is not necessary and default closeOnBackdropClick works. If in the adopting phase, we realize that users are relying on the DOM structure, we can easily return the div back, but we will have to introduce a logic, that will close the modal if we click on this container, but not on the paper component

TomasSlama · 2023-08-11T17:42:32Z

packages/picasso/src/PromptModal/PromptModal.tsx

-    } catch (err) {
-      setError(true)
-      setLoading(false)
+export const PromptModal = forwardRef<HTMLDivElement, Props>(


The only change is

- HTMLElement + HTMLDivElement

And the removal of the styles, as they were empty anyway

TomasSlama · 2023-09-14T15:08:57Z

@toptal-bot run package:alpha-release

toptal-devbot · 2023-09-14T15:16:05Z

Your alpha package is ready 🎉
yarn add @topkit/analytics-charts@49.0.1-alpha-fx-4034-6b0ba2a89.28+6b0ba2a89
yarn add @toptal/picasso@38.2.1-alpha-fx-4034-6b0ba2a89.9+6b0ba2a89
yarn add @toptal/picasso-charts@52.0.1-alpha-fx-4034-6b0ba2a89.28+6b0ba2a89
yarn add @toptal/picasso-codemod@5.5.3-alpha-fx-4034-6b0ba2a89.135+6b0ba2a89
yarn add @toptal/picasso-forms@61.1.3-alpha-fx-4034-6b0ba2a89.7+6b0ba2a89
yarn add @toptal/picasso-pictograms@1.1.2-alpha-fx-4034-6b0ba2a89.210+6b0ba2a89
yarn add @toptal/picasso-provider@3.2.1-alpha-fx-4034-6b0ba2a89.28+6b0ba2a89
yarn add @toptal/picasso-rich-text-editor@6.0.3-alpha-fx-4034-6b0ba2a89.23+6b0ba2a89
yarn add @toptal/picasso-shared@12.0.1-alpha-fx-4034-6b0ba2a89.255+6b0ba2a89

dmaklygin · 2023-11-15T09:58:37Z

@toptal-bot run package:alpha-release

toptal-devbot · 2023-11-15T10:02:53Z

Your alpha package is ready 🎉
yarn add @topkit/analytics-charts@52.0.1-alpha-fx-4034-e23089f0e.58+e23089f0e
yarn add @toptal/picasso@41.1.1-alpha-fx-4034-e23089f0e.11+e23089f0e
yarn add @toptal/picasso-charts@55.1.1-alpha-fx-4034-e23089f0e.20+e23089f0e
yarn add @toptal/picasso-codemod@5.6.3-alpha-fx-4034-e23089f0e.61+e23089f0e
yarn add @toptal/picasso-forms@65.0.2-alpha-fx-4034-e23089f0e.29+e23089f0e
yarn add @toptal/picasso-pictograms@2.0.1-alpha-fx-4034-e23089f0e.79+e23089f0e
yarn add @toptal/picasso-provider@3.4.2-alpha-fx-4034-e23089f0e.70+e23089f0e
yarn add @toptal/picasso-query-builder@1.1.2-alpha-fx-4034-e23089f0e.22+e23089f0e
yarn add @toptal/picasso-rich-text-editor@9.0.2-alpha-fx-4034-e23089f0e.29+e23089f0e
yarn add @toptal/picasso-shared@13.0.1-alpha-fx-4034-e23089f0e.79+e23089f0e

TomasSlama self-assigned this Aug 11, 2023

TomasSlama commented Aug 11, 2023

View reviewed changes

toptal-devbot temporarily deployed to staging August 11, 2023 17:43 Inactive

toptal deleted a comment from toptal-devbot Sep 14, 2023

TomasSlama force-pushed the fx-4034 branch from 0a7c81a to 6b0ba2a Compare September 14, 2023 15:07

dmaklygin and others added 12 commits November 15, 2023 12:25

chore: install tailwindcss and @mui/base

bce1466

chore: install tailwindcss and @mui/base

9312c97

chore: setup colors and screens in tailwind

e11b2ce

chore: add BASE typography and shadows

897e579

refactor: migrate Modal to Tailwindcss

d20fb98

feat: migrate Drawer to TailwindCSS

1dd31e5

feat: migrate Modal compound components to TailwindCSS

19a3728

chore: add negative tab index to drawer paper

da9588c

chore: remove logs

e0ff703

chore: add proper aria label to the close button

d66b98c

chore: update yarn.lock

d2246eb

chore: update Modal

e23089f

dmaklygin force-pushed the fx-4034 branch from 1e10af8 to e23089f Compare November 15, 2023 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate Modal to TailwindCSS #3800

refactor: migrate Modal to TailwindCSS #3800

TomasSlama commented Aug 11, 2023 •

edited by jira bot

changeset-bot bot commented Aug 11, 2023 •

edited

TomasSlama Aug 11, 2023

TomasSlama Aug 11, 2023

TomasSlama Aug 11, 2023

TomasSlama Aug 11, 2023

TomasSlama Aug 11, 2023

TomasSlama commented Sep 14, 2023

toptal-devbot commented Sep 14, 2023

dmaklygin commented Nov 15, 2023

toptal-devbot commented Nov 15, 2023

refactor: migrate Modal to TailwindCSS #3800

Are you sure you want to change the base?

refactor: migrate Modal to TailwindCSS #3800

Conversation

TomasSlama commented Aug 11, 2023 • edited by jira bot

Description

How to test

Screenshots

Development checks

When to approve? ✅

When to request changes? ❌

When to comment (neither ✅ nor ❌)

How to handle the comments?

changeset-bot bot commented Aug 11, 2023 • edited

⚠️ No Changeset found

TomasSlama Aug 11, 2023

Choose a reason for hiding this comment

TomasSlama Aug 11, 2023

Choose a reason for hiding this comment

TomasSlama Aug 11, 2023

Choose a reason for hiding this comment

TomasSlama Aug 11, 2023

Choose a reason for hiding this comment

TomasSlama Aug 11, 2023

Choose a reason for hiding this comment

TomasSlama commented Sep 14, 2023

toptal-devbot commented Sep 14, 2023

dmaklygin commented Nov 15, 2023

toptal-devbot commented Nov 15, 2023

TomasSlama commented Aug 11, 2023 •

edited by jira bot

changeset-bot bot commented Aug 11, 2023 •

edited