🚀 refactor(drawer): adjust drawer styles and structure by matheushdsbr · Pull Request #647 · gympass/yoga

matheushdsbr · 2023-04-06T19:03:56Z

Description 📄

In this PR I'm refactoring the drawer and adding some new props.

I realized that the Drawer receives the body of the Dialog as a base, but I needed to add some props to the <Drawer.Header>, so I thought about refactoring the <Dialog.Header> by changing a little how it works. This change also affected the <BottomSheet.Header>.

The <Drawer.Header> now has the option to onClose, backHandler and title.

onClose and backHandler are very similar, they receive a function, but each with its own icon:

onClose -> Close icon,
backHandler -> ArrowLeft icon,
title -> string

To use <Drawer.Header> just use its props.

This PR will trigger a breaking change <---
Need to adjust components that are using <Dialog.Header> and <BottomSheet.Header>

Obs:

Added @testing-library/user-event and @testing-library/dom package to perform click actions on tests with userEvent as recommended by testing library doc

Platforms 📲

Web
Mobile

Type of change 🔍

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested? 🧪

[Enter the tips to test this PR]

Unit Test
Snapshot Test

Checklist: 🔍

Screenshots 📸

Before	After

With back button

With divider on header

With no close button

With no header

caiotracera · 2023-04-13T23:31:03Z

Hey, @matheushdsbr!
Congrats for the initiative! I am wondering if this PR fixes #632 🤔

matheushdsbr · 2023-04-14T17:21:12Z

Hey, @matheushdsbr! Congrats for the initiative! I am wondering if this PR fixes #632 🤔

Thanks! Not yet, but thanks for mentioning it. I will analyze the situation

matheushdsbr · 2023-04-14T17:27:10Z

Hey @caiotracera, I believe that to solve this bug it would be better to open another PR, the purpose of this one at first is to adjust the structure of the drawer. What do you think?

…eader

…t, adjust tests

…dler

… borders

MicaelRodrigues · 2023-05-02T19:35:10Z

+  function showCloseButton() {
+    if (onClose && !hideCloseButton) {
+      return (
+        <Heading.RightButton
+          aria-label="close-button-drawer"
+          onClick={onClose}
+          icon={Close}
+          large
+        />
+      );
+    }
+
+    return null;
+  }
+
+  function showTitle() {
+    if (title) {
+      return (
+        <Heading.Title
+          style={{
+            letterSpacing: '1px',
+            textTransform: 'uppercase',
+            fontSize: '12px',
+          }}
+        >
+          {title}
+        </Heading.Title>
+      );
+    }
+
+    return null;
+  }
+
+  function showBackButton() {
+    if (backHandler) {
+      return (
+        <Heading.BackButton
+          aria-label="back-button-drawer"
+          onClick={backHandler}
+        />
+      );
+    }
+
+    return null;
+  }
+
+  function showDivider() {
+    if (divider) {
+      return (
+        <Box marginTop="small">
+          <Divider aria-label="divider-drawer" />
+        </Box>
+      );
+    }
+
+    return null;
+  }


Defining components within components is a bad practise.

Explanation why:
https://youtu.be/QuLfCUh-iwI?t=532

MicaelRodrigues · 2023-05-02T19:39:13Z

+  return (
+    <Box width="100%">
+      <Box paddingTop="small" paddingRight="small" paddingLeft="xxlarge">
+        <Heading noPadding role="heading" aria-label="header-drawer">


Don't define a fixed aria-label. You defining a "meaning" that not explain the usage of the "Heading". If really needed (which i don't think it ) it should be costumizable.

MicaelRodrigues · 2023-05-02T19:41:31Z

+  padding: ${theme.spacing.zero} !important;
+  border-radius: ${theme.borders.zero} !important;


If you are using !important here probably something is not right with the css structure.
Can you check?

MicaelRodrigues · 2023-05-02T19:44:31Z

-  /** Hide the close button when onClose prop is defined. */
-  hideCloseButton: bool,


Why did you remove hideCloseButton prop? It will break some implementations that are using it.
Example, in partners portal:

https://github.com/gympass/partners-reports-mfe/blob/master/src/components/templates/DrawerPage/DrawerPage.tsx

MicaelRodrigues · 2023-05-02T19:51:56Z

-  hideCloseButton: bool,
  /** Function to close the dialog. */
  onClose: func,
+  backHandler: func,


Following the name convention of "onClose", WDYT of onBack here?

MicaelRodrigues · 2023-05-08T12:39:08Z

+
+  const BoxContent = styled(Box)`
+    height: 100%;
+    max-width: 340px;


We should not assume these sizes. The box should be always 100% of Drawer.Content. and Drawer.Content should be the one setting the spacing

matheushdsbr self-assigned this Apr 6, 2023

github-actions Bot added @gympass/yoga @gympass/yoga-doc labels Apr 6, 2023

matheushdsbr marked this pull request as ready for review April 11, 2023 13:53

github-actions Bot added the @gympass/yoga-labnative label Apr 11, 2023

matheushdsbr marked this pull request as draft April 11, 2023 18:42

matheushdsbr marked this pull request as ready for review April 11, 2023 19:15

luanlorenzo reviewed Apr 12, 2023

View reviewed changes

Comment thread packages/yoga/src/Drawer/web/Header.jsx

matheushdsbr added 19 commits May 2, 2023 15:16

refactor(drawer): adjust structure and styles

8c43a13

refactor(drawer): remove drawer props of dailog and bring to drawer.h…

7dde687

…eader

style(drawer): adjust styles

c2745a8

docs(drawer): add drawer.header props

aaabd78

refactor(drawer): adjust the order of props

857f8d5

style(drawer): add margin-top to divider

764d2bb

test(drawer): add @testing-library/dom and @testing-library/user-even…

4edeb87

…t, adjust tests

test(drawer): add tests to drawer.header

42e85ca

refactor(drawer|dialog): adjust props name

5c68f0e

test(drawer): add aria-label to tests and adjust prop name to backHan…

8a2a9c3

…dler

test(drawer): add tests to drawer.header

7cdc84a

refactor(drawer): adjust breakline and add !importants to padding and…

c1c6ca1

… borders

refactor(dialog): refactor dialog.header to add close button in it

a6e5320

docs(dialog): add dialog.header props

7da4d63

test(drawer): update snapshot

f68cf04

test(bottomsheet): adjust test show bottom onclose

dcce8d7

refactor(drawer): add heading component to adjust drawer.head structure

76a8e9e

test(drawer): add aria-label and update snapshot

d8ffac1

chore(drawer): update yarn.lock

0e4b902

matheushdsbr force-pushed the refactor/adjust-drawer branch from 71d3a54 to 0e4b902 Compare May 2, 2023 18:26

MicaelRodrigues reviewed May 2, 2023

View reviewed changes

MicaelRodrigues reviewed May 8, 2023

View reviewed changes

psidneis requested review from ericcleao and tcK1 May 8, 2023 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 refactor(drawer): adjust drawer styles and structure#647

🚀 refactor(drawer): adjust drawer styles and structure#647
matheushdsbr wants to merge 19 commits intomasterfrom
refactor/adjust-drawer

matheushdsbr commented Apr 6, 2023 •

edited

Loading

Uh oh!

Uh oh!

caiotracera commented Apr 13, 2023

Uh oh!

matheushdsbr commented Apr 14, 2023

Uh oh!

matheushdsbr commented Apr 14, 2023

Uh oh!

MicaelRodrigues May 2, 2023

Uh oh!

MicaelRodrigues May 2, 2023

Uh oh!

MicaelRodrigues May 2, 2023

Uh oh!

MicaelRodrigues May 2, 2023 •

edited

Loading

Uh oh!

MicaelRodrigues May 2, 2023

Uh oh!

MicaelRodrigues May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		padding: ${theme.spacing.zero} !important;
		border-radius: ${theme.borders.zero} !important;

		/** Hide the close button when onClose prop is defined. */
		hideCloseButton: bool,

Conversation

matheushdsbr commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📄

Platforms 📲

Type of change 🔍

How Has This Been Tested? 🧪

Checklist: 🔍

Screenshots 📸

With back button

With divider on header

With no close button

With no header

Uh oh!

Uh oh!

caiotracera commented Apr 13, 2023

Uh oh!

matheushdsbr commented Apr 14, 2023

Uh oh!

matheushdsbr commented Apr 14, 2023

Uh oh!

MicaelRodrigues May 2, 2023

Choose a reason for hiding this comment

Uh oh!

MicaelRodrigues May 2, 2023

Choose a reason for hiding this comment

Uh oh!

MicaelRodrigues May 2, 2023

Choose a reason for hiding this comment

Uh oh!

MicaelRodrigues May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MicaelRodrigues May 2, 2023

Choose a reason for hiding this comment

Uh oh!

MicaelRodrigues May 8, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matheushdsbr commented Apr 6, 2023 •

edited

Loading

MicaelRodrigues May 2, 2023 •

edited

Loading