Conversation
|
@Philip-Cheung these components are feature-complete(ish), but the design is largely judgement calls and mimicking our general design language. Please take a look to familiarize yourself with these components' feature-set and behaviour; when you have a formal design spec, I'll work back on that. @ty2k would appreciate an initial look over this, particularly the Menu piece. I've taken a slightly different approach to implementation than we did with Select (ie. decomposing into multiple discrete subcomponents), and hit on a bunch of complexity around how to elegantly handle multiple methods of composition ( |
|
@mkernohanbc will take a look at this this afternoon |
This tracked back to a discrepancy between FA versions, corrected in 882a9a3 |
ty2k
left a comment
There was a problem hiding this comment.
Very slick! I like the Menu implementation and it seems to work really nicely with keyboard controls, including for complex cases like the Submenu.
I haven't gone through this with a fine-toothed comb because of the amount of code here, but this is a first pass. In general, I think it would be ideal to break this into several PRs:
- One for each SVG icon
- Popover by itself if it's generic
- Menu and MenuItem
- Navbar
|
|
||
| return ( | ||
| <div className="bcds-react-aria-Navbar"> | ||
| <Toolbar |
There was a problem hiding this comment.
I think Toolbar should just be a regular <nav>.
The issue with Toolbar is it gets the toolbar role, whereas a <nav> gets navigation.
|
|
||
| import "./Popover.css"; | ||
|
|
||
| export default function Popover(props: PopoverProps) { |
There was a problem hiding this comment.
Is this a general purpose Popover or specific to Navbar and Menu? Can it go in by itself if it's generic? If it's generic, we're already using the Popover from RAC throughout the library, so the names and usage should be reconciled.
If it's specific, can it be renamed for specificity?
| @@ -1,5 +1,5 @@ | |||
| import { cloneElement, PropsWithChildren } from "react"; | |||
|
|
|||
| import Navbar from "../Navbar"; | |||
There was a problem hiding this comment.
Can we leave it out of Header?
- Better separation of concerns.
- Leaves the
Headerprops cleaner. - Better customization for breakpoints (contrived example: Header goes from desktop to tablet design at 900px but Navbar needs to change at 750px).
| width="10" | ||
| height="10" | ||
| viewBox="0 0 10 10" | ||
| viewBox="0 0 448 512" |
There was a problem hiding this comment.
I see the move to the v6 version of this icon has a different (not square) aspect ratio. Will this cause visual regressions in other components like Checkbox?
Something to think about in general for icons if we're going to mix and match versions of Font Awesome. I think the majority we have right now are following the old square ratio.


Draft PR for initial feedback on design and implementation.
This PR adds two new components:
Menu
Menu is logically very similar to Select, and follows the same Collections API.
The
MenuTrigger,MenuandMenuItemcomponents are the core parts, butMenuSection,MenuSectionandSubmenuTriggercomponents are also exported to support complex composition:Menus can be composed dynamically using the
itemsorsectionsprops, or manually composed via thechildrenslot.See Storybook docs for a more detailed explanation of the structure of this set of components, and each component's individual API.
Navbar
This is a (currently extremely basic) implementation of an idea we've discussed previously: a secondary navigation menu that can be used alongside the Header component.
What I currently have on this branch is:
subMenuItemspropNavbar has relatively little internal logic of its own, beyond sizing/spacing and automatically generating
<Separator />elements between items. It is built around a genericchildrenslot, expecting some number of<Link>,<Button>,<Menu>etc.