-
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
add sorting header for mobile file browser #1786
Changes from 12 commits
4dfc854
cd9ba62
6bc046a
86e580b
4e1475f
2e1a0ed
6b37ee0
31f7b51
9442fc4
7a8c0b4
ca3d282
e32dbc5
b502c4e
1a59752
b5d66c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import * as React from "react" | ||
import createSvgIcon from "../createSvgIcon" | ||
import { ReactComponent as SortSvg } from "../svgs/sort.svg" | ||
|
||
export { SortSvg } | ||
|
||
export default createSvgIcon(<SortSvg />) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
import React, { useState, ReactNode, useMemo } from "react" | ||
import { Menu as MaterialMenu, MenuItem } from "@material-ui/core" | ||
import { List, ListItem, Menu as MaterialMenu, PopoverOrigin } from "@material-ui/core" | ||
import { makeStyles, createStyles } from "@chainsafe/common-theme" | ||
import clsx from "clsx" | ||
import { useCallback } from "react" | ||
import { CSFTheme } from "../Themes/types" | ||
|
||
interface Option { | ||
contents: ReactNode | ||
inset?: boolean | ||
testId?: string | ||
onClick?: () => void | ||
} | ||
|
||
|
@@ -22,6 +24,8 @@ interface Props { | |
options: Option[] | ||
style?: CustomClasses | ||
testId?: string | ||
anchorOrigin?: PopoverOrigin | ||
transformOrigin?: PopoverOrigin | ||
} | ||
|
||
const useStyles = makeStyles(({ constants }: CSFTheme) => { | ||
|
@@ -40,7 +44,7 @@ const useStyles = makeStyles(({ constants }: CSFTheme) => { | |
} | ||
})}) | ||
|
||
export default function Menu({ icon, options, style, testId }: Props) { | ||
export default function Menu({ icon, options, style, testId, anchorOrigin, transformOrigin }: Props) { | ||
const [anchorEl, setAnchorEl] = useState(null) | ||
const open = useMemo(() => Boolean(anchorEl), [anchorEl]) | ||
const classes = useStyles() | ||
|
@@ -68,20 +72,24 @@ export default function Menu({ icon, options, style, testId }: Props) { | |
open={open} | ||
onClose={handleClose} | ||
PopoverClasses={{ paper: classes.paper, root: style?.root }} | ||
anchorOrigin={anchorOrigin} | ||
transformOrigin={transformOrigin} | ||
> | ||
{options.map((option, index) => ( | ||
<MenuItem | ||
key={index} | ||
onClick={() => { | ||
handleClose() | ||
option.onClick && option.onClick() | ||
}} | ||
focusVisibleClassName={clsx(style?.focusVisible)} | ||
className={classes.options} | ||
> | ||
{option.contents} | ||
</MenuItem> | ||
))} | ||
<List> | ||
{options.map((option, index) => ( | ||
<ListItem | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what this adds, but the example from mui does not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The example you referenced is for MUI v5. See here for documentation: https://v4.mui.com/components/menus/ List and MenuList are nearly identical, with MenuList just providing some accessibility benefits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either case, I took a look at the markup generated, and Menu already handles this internally, so I have removed the |
||
key={index} | ||
onClick={() => { | ||
option.onClick && handleClose() | ||
option.onClick && option.onClick() | ||
}} | ||
focusVisibleClassName={clsx(style?.focusVisible)} | ||
className={classes.options} | ||
> | ||
{option.contents} | ||
</ListItem> | ||
))} | ||
</List> | ||
</MaterialMenu> | ||
</div> | ||
) | ||
|
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.
Do we really need to use the material imports here if we want to get rid of the dependency in Files ultimately?
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.
It saves us from having to reimplement the
inset
prop. This would just be an update of where the component is imported from, once we actually move forward with the upgrade.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.
Hehe yeah that'll probably be on me, hence my complaints 😅