-
Notifications
You must be signed in to change notification settings - Fork 21
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/migrate modals #401
Feat/migrate modals #401
Conversation
Fix type errors
Move storybook test libs to dev dependencies
@@ -44,6 +44,7 @@ export const Button: FC<Props> = ({ | |||
const component = ( | |||
<RebassButton | |||
variant={intent} | |||
as={href ? 'span' : 'button'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both a Button and a tag are focusable components so using the keyboard Tab would visit the same component twice, this is to remove button when we have a tag
onClose: any; | ||
title: string | ReactNode; | ||
content: string | ReactNode; | ||
title: ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReacNode type includes string
@@ -12,7 +11,6 @@ import { INotification, createNotifier, useNotifier } from './notifier'; | |||
|
|||
const meta: Meta<INotification> = { | |||
title: 'Notifier', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now autodoc is enabled globally
@@ -22,77 +29,66 @@ export interface DrawerProps extends Omit<PopupProps, 'css'> { | |||
children: Children | Children[]; | |||
isOpen?: boolean; | |||
singleBottom?: boolean; | |||
onClose?: () => void; | |||
onClose: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a breaking change on Hopsworks side since we are passing it on every usage
right?: string; | ||
isOpen?: boolean; | ||
onBackdropClick?: (event: React.SyntheticEvent<HTMLDivElement>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was no usage on Hopsworks
onClose = () => {}, | ||
hasCloseButton = false, | ||
backdropSx = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been replaced with overlayProps
':focus-visible': { | ||
boxShadow: '0 0 0 3px rgba(33, 177, 130, 0.6)', | ||
_dark: { | ||
boxShadow: '0 0 0 3px rgba(33, 177, 130, 0.6)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might need to change boxShadow for dark mode
const { definePartsStyle, defineMultiStyleConfig } = | ||
createMultiStyleConfigHelpers(parts.keys); | ||
|
||
const xl7 = defineStyle({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
custom size for making Popups look as before
maxHeight: 'calc(100vh - 80px)', | ||
}); | ||
|
||
const drawer = definePartsStyle({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a very exceptional variant just used for feature basket on Hopsworks
There are no API breaking changes but the styling has breaking changes (eg if we don't pass the correct size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code-wise looks purely amazing!
there is this issue with the popup requiring an additional click on it to get proper focus for using esc/tab keys
package.json
Outdated
"prepack": "run-s build", | ||
"gen:cahkra-theme-typings": "chakra-cli tokens src/theme-chakra/theme/index.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pure gold ❤️
(there is a typo mentioned above)
# [2.7.0](v2.6.1...v2.7.0) (2023-05-23) ### Features * **modals:** Migrate Popup, Drawer, TinyPopup to Chakra ([#401](#401)) ([3e3615f](3e3615f))
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Some package upgrades
Separating Stories for Popup, TinyPopup, and Drawer
Migrating Popup, TinyPopup, and Drawer to use Chakra built-in Modal and Drawer
There are some minor breaking changes for the styling of Modals the would only accept size