Skip to content
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.

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

feat(Drawer): new component #1306

Merged
merged 19 commits into from
Jan 9, 2025
Merged

feat(Drawer): new component #1306

merged 19 commits into from
Jan 9, 2025

Conversation

pladaria
Copy link
Member

@pladaria pladaria commented Dec 16, 2024

@pladaria pladaria changed the title deat(Drawer): new component feat(Drawer): new component Dec 16, 2024
Copy link

github-actions bot commented Dec 16, 2024

Size stats

master this branch diff
Total JS 12.3 MB 12.3 MB +11.7 kB
JS without icons 2.04 MB 2.05 MB +11.6 kB
Lib overhead 72.8 kB 72.8 kB 0 B
Lib overhead (gzip) 17.1 kB 17.1 kB 0 B

Copy link

github-actions bot commented Dec 16, 2024

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

Copy link

github-actions bot commented Dec 16, 2024

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-p7cyucqc7-flows-projects-65bb050e.vercel.app

Built with commit 37af599.
This pull request is being automatically deployed with vercel-action

@@ -20,29 +20,22 @@ export const Portal = ({children, className}: Props): JSX.Element | null => {
const [container, setContainer] = React.useState<HTMLDivElement | null>(null);
Copy link
Member Author

@pladaria pladaria Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous implementation was adding two nodes (because of strict mode in dev) and removing one, leaving leftovers in the dom.

src/drawer.css.ts Outdated Show resolved Hide resolved
@pladaria pladaria marked this pull request as ready for review December 20, 2024 11:41
checkTestIds(<Drawer title="Title" subtitle="Subtitle" description="Description" onClose={() => {}} />, [
{
componentName: 'Drawer',
internalTestIds: ['title', 'subtitle', 'description'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these defined in the spec?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in spec, but I was assuming the same testids for these elements that we were using in cards, etc

src/drawer.css.ts Outdated Show resolved Hide resolved
}, [onClose]);

const dismiss = React.useCallback(() => {
return close().then(() => onDismiss?.());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between onDismiss and onClose? I initially thought that onDismiss was called immediately and onClose when the animation ends, but it seems both are called when the animation ends :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I see the difference. onDismiss is only called when the drawer is closed by a dismiss action (esc key /close button, click outside). But can't we call onDismiss immediately without waiting for animation?

Copy link
Member Author

@pladaria pladaria Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could call it immediately but I decided to not emit the close/dismiss event while the drawer is visible.

If you want to make some action on dismiss/close/action, you could be visually missing that change if it happens behind the drawer

)}
/>
<div
data-testid="drawerLayout"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this testid needed?

Copy link
Member Author

@pladaria pladaria Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. I've added it to differentiate the boundaries of the different parts of the Drawer composite. The DrawerLayout is a separate component as Drawer.

Can be removed if you want to.

src/drawer.tsx Show resolved Hide resolved
src/drawer.css.ts Outdated Show resolved Hide resolved
src/drawer.css.ts Outdated Show resolved Hide resolved
src/drawer.css.ts Outdated Show resolved Hide resolved
},
});

export const overlay = style([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aweell I think the overlay transition is not needed in mobile. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. And If the drawer is white over a white background, it adds contrast.

@pladaria pladaria requested a review from yceballost December 20, 2024 17:41
src/drawer.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@aweell aweell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

Just as a nice to have last comment... if we can set the min value of the storybook width slider to the min-width of the drawer would be awesome!

Screenshot 2025-01-08 at 14 26 26

@pladaria pladaria enabled auto-merge January 9, 2025 09:35
@pladaria pladaria disabled auto-merge January 9, 2025 09:37
@pladaria
Copy link
Member Author

pladaria commented Jan 9, 2025

Nice work!

Just as a nice to have last comment... if we can set the min value of the storybook width slider to the min-width of the drawer would be awesome!

Screenshot 2025-01-08 at 14 26 26

set to 300, so the min-width style can be verified

@pladaria pladaria enabled auto-merge January 9, 2025 09:41
@pladaria pladaria added this pull request to the merge queue Jan 9, 2025
Merged via the queue into master with commit 2543c67 Jan 9, 2025
11 checks passed
@pladaria pladaria deleted the pladaria/WEB-2113_drawer branch January 9, 2025 09:57
tuentisre pushed a commit that referenced this pull request Jan 9, 2025
# [16.8.0](v16.7.0...v16.8.0) (2025-01-09)

### Bug Fixes

* **react-datetime:** upgrade react-datetime to support React18 ([#1303](#1303)) ([9caf2c2](9caf2c2))

### Features

* **Chip:** change background color on hover/active ([#1302](#1302)) ([73e33eb](73e33eb))
* **Drawer:** new component ([#1306](#1306)) ([2543c67](2543c67))
* **icons:** New floppy disk icon ([#1304](#1304)) ([d9c5353](d9c5353)), closes [/github.com/Telefonica/mistica-design/issues/1956#issuecomment-2535956321](https://github.com//github.com/Telefonica/mistica-design/issues/1956/issues/issuecomment-2535956321)
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 16.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants