-
Notifications
You must be signed in to change notification settings - Fork 16
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
Refactor export menu with new headless UI library #1567
Conversation
import styles from './Selector/Selector.module.css'; | ||
|
||
interface Props { | ||
format: string; | ||
url: URL | (() => Promise<URL | Blob>) | undefined; | ||
} | ||
|
||
function ExportEntry(props: Props) { | ||
const { format, url } = props; | ||
const ExportEntry = forwardRef< |
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 render
components need to be open for extension (i.e. forward ref
and spread extra props).
} | ||
|
||
function ExportMenu(props: Props) { | ||
const { entries, isSlice, align = 'center' } = props; | ||
|
||
return ( | ||
<Wrapper className={styles.wrapper}> | ||
<Button | ||
<MenuProvider placement={PLACEMENTS[align]}> |
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.
No need to do this ourselves anymore.
@@ -108,10 +111,12 @@ | |||
} | |||
|
|||
.option:hover, | |||
.option[data-active-item], |
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 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.
.option:hover
and .option:focus
will probably be removed once I finish refactoring the other menus.
I'll investigate the strange failing tests tomorrow... |
e9adba2
to
6367288
Compare
/approve |
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.
Upgrading to the latest version of ariakit
solved the error I was having in the tests.
.exportMenu { | ||
display: flex; | ||
flex-direction: column; | ||
min-width: var(--popover-anchor-width); | ||
margin-top: 0.375rem; | ||
padding: 0.25rem 0; | ||
background-color: var(--h5w-selector-menu--bgColor, white); | ||
overflow: hidden scroll; | ||
scrollbar-width: thin; | ||
box-shadow: | ||
rgba(0, 0, 0, 0.1) 0px 0px 0px 1px, | ||
rgba(0, 0, 0, 0.1) 0px 4px 11px; | ||
outline: none; | ||
} |
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 will probably be shared with the other menus once I've refactored them.
@@ -108,10 +111,12 @@ | |||
} | |||
|
|||
.option:hover, | |||
.option[data-active-item], |
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.
.option:hover
and .option:focus
will probably be removed once I finish refactoring the other menus.
Sweet ! |
On hold, as I noticed that this PR increases the bundle size by more than 100 kB, which seems highly unreasonable for just a popover menu. I'll investigate. |
I've tried with React Aria (in branch Mind blowing... |
Superseded by #1642 |
react-aria-menubutton
is no longer maintained. It is still on React 17 and CommonJS, which is causing issues in our Cypress tests and for consumer applications.In this PR, I'm starting the migration to another headless UI library called Ariakit, which seems like a well-maintained, featureful, Typescript-first library. I've considered and discarded other options:
I'll continue the migration in separate PRs.