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

chore: add drawer component #3294

Merged
merged 16 commits into from
Nov 20, 2023
Merged

chore: add drawer component #3294

merged 16 commits into from
Nov 20, 2023

Conversation

YahiaElTai
Copy link
Contributor

@YahiaElTai YahiaElTai commented Nov 8, 2023

Summary

This PR adds the new Drawer component which will implement a side panel container with similarities to modal pages.

image

Copy link
Contributor

github-actions bot commented Nov 8, 2023

Deploy preview for merchant-center-application-kit ready!

✅ Preview
https://merchant-center-application-876fr84ll-commercetools.vercel.app
https://appkit-sha-ac20fb954c1c56bb1f382b65af946bdfd1fd67be.commercetools.vercel.app
https://appkit-pr-3294.commercetools.vercel.app

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

Copy link
Contributor

github-actions bot commented Nov 8, 2023

Deploy preview for application-kit-custom-views ready!

✅ Preview
https://application-kit-custom-views-1x2pttih6-commercetools.vercel.app
https://appkit-cv-sha-ac20fb954c1c56bb1f382b65af946bdfd1fd67be.commercetools.vercel.app
https://appkit-cv-pr-3294.commercetools.vercel.app

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

@YahiaElTai YahiaElTai self-assigned this Nov 8, 2023
@YahiaElTai YahiaElTai added the 🙏 Status: Dev Review Waiting for technical reviews label Nov 9, 2023
Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: f3b3a78

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

This PR includes changesets to release 37 packages
Name Type
@commercetools-frontend/application-components Minor
@commercetools-local/visual-testing-app Minor
@commercetools-local/playground Minor
@commercetools-frontend/application-shell Minor
@commercetools-frontend/codemod Minor
@commercetools-frontend/mc-scripts Minor
@commercetools-applications/merchant-center-template-starter-typescript Minor
@commercetools-applications/merchant-center-template-starter Minor
@commercetools-applications/merchant-center-custom-view-template-starter-typescript Minor
@commercetools-website/components-playground Minor
@commercetools-frontend/cypress Minor
@commercetools-backend/eslint-config-node Minor
@commercetools-backend/express Minor
@commercetools-backend/loggers Minor
@commercetools-frontend/actions-global Minor
@commercetools-frontend/application-config Minor
@commercetools-frontend/application-shell-connectors Minor
@commercetools-frontend/assets Minor
@commercetools-frontend/babel-preset-mc-app Minor
@commercetools-frontend/browser-history Minor
@commercetools-frontend/constants Minor
@commercetools-frontend/create-mc-app Minor
@commercetools-frontend/eslint-config-mc-app Minor
@commercetools-frontend/i18n Minor
@commercetools-frontend/jest-preset-mc-app Minor
@commercetools-frontend/jest-stylelint-runner Minor
@commercetools-frontend/l10n Minor
@commercetools-frontend/mc-dev-authentication Minor
@commercetools-frontend/mc-html-template Minor
@commercetools-frontend/notifications Minor
@commercetools-frontend/permissions Minor
@commercetools-frontend/react-notifications Minor
@commercetools-frontend/sdk Minor
@commercetools-frontend/sentry Minor
@commercetools-frontend/url-utils Minor
@commercetools-website/custom-applications Minor
@commercetools-website/custom-views Minor

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

@CarlosCortizasCT
Copy link
Contributor

Thanks @YahiaElTai, this looks great! 👍

Maybe just a couple of things more to play safe in the long term:

  • Maybe we could add another VRT where both title and subtitle are so long they will use multiple lines: this would help to make sure those respect the close icon "column".
  • Since the Drawer has intro and exit animations, it would be great to add an example to the playground application so we can validate it. This would be the file to update. Please let me know if you need some context about how the playground works.

Also for @FilPob, since you can't see the new component in a preview deployment, just wanted to share that the new overlay color seems too shallow to me and just wanted to double check it is intended 😅

image

@FilPob
Copy link

FilPob commented Nov 14, 2023

@CarlosCortizasCT thanks for the early update! Yes the light overlay is expected.

@YahiaElTai
Copy link
Contributor Author

Thanks @YahiaElTai, this looks great! 👍

Maybe just a couple of things more to play safe in the long term:

  • Maybe we could add another VRT where both title and subtitle are so long they will use multiple lines: this would help to make sure those respect the close icon "column".
  • Since the Drawer has intro and exit animations, it would be great to add an example to the playground application so we can validate it. This would be the file to update. Please let me know if you need some context about how the playground works.

Also for @FilPob, since you can't see the new component in a preview deployment, just wanted to share that the new overlay color seems too shallow to me and just wanted to double check it is intended 😅

image

Thanks @CarlosCortizasCT for all the feedback :) much appreciated.

  • I have added the requested VRT for title and subtitle. (I also removed the truncate so now the title and subtitle both will stretch on multiple lines)
  • I have added the new example to the playground
image

@emmenko
Copy link
Member

emmenko commented Nov 14, 2023

Thanks @YahiaElTai for the work so far ❤️

A couple of general questions:

  • Is it allowed to open another drawer on top of a drawer? If so, what's the current / expected behaviour?
  • Is it allowed to open a modal component (e.g. dialog) on top of a drawer? If so, what's the current / expected behaviour?

Maybe you can use the playground to check / verify these points. Thanks 🤗

Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT left a comment

Choose a reason for hiding this comment

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

Could you please move the PR out of draft mode?

@YahiaElTai YahiaElTai marked this pull request as ready for review November 17, 2023 10:08
@YahiaElTai
Copy link
Contributor Author

@CarlosCortizasCT Thanks for the comments and the help with this PR. The only thing left is to review and approve the percy snapshots. I will leave that to you 😄

Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT left a comment

Choose a reason for hiding this comment

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

Ok. Now it looks great to me 💯

Thanks a lot for your awesome work 🙇

@CarlosCortizasCT CarlosCortizasCT requested a review from a team November 17, 2023 13:09
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution! ❤️

Only a changeset is missing now 😉

};

export const getContainerStyles = (_props: StyleProps): SerializedStyles => css`
position: absolute;
top: 0;
right: 0;
height: 100%;
width: ${_props.size === 'small' ? '600px !important' : '100%'};
width: ${_props.size !== 'scale'
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I know it was already like this but could you rename it to props instead of _props? In TS an underscore as a prefix is used to silent the compiler that a variable is unused.

Thanks

Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

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

💯

@CarlosCortizasCT CarlosCortizasCT merged commit db6e172 into main Nov 20, 2023
14 checks passed
@CarlosCortizasCT CarlosCortizasCT deleted the CKT-2182-drawer-component branch November 20, 2023 11:56
@ct-changesets ct-changesets bot mentioned this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙏 Status: Dev Review Waiting for technical reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants