Open adrian-reapit opened 1 month ago
Hi @adrian-reapit and @kurtdoherty, I've been thinking about splitting this ticket into at least 3 PRs:
Here’s my understanding of the Nav Icon Item and Bottom Bar Item component based on the design, along with my proposed code.
<a>
elements and <button>
elementsBy Looking at Button component in Figma. I see some differences:
Additionally, there are a few features that Button component doesn't have:
Because of this, I’m thinking of building the Nav Icon Item and Bottom Bar Icon from scratch and I'll put them under AppBar folder Below is the code snippet I propose:
const NavIconItem = ({ icon, isActive = false, badge, tooltip, onClick, href, ...rest }) => {
return (
<NavItemContainer data-active={isActive} title={tooltip} onClick={onClick} href={href} {...rest}>
<IconContainer>
{isValidElement(icon) ? <>{icon}</> : <Icon icon={icon} />}
{{isValidElement(badge) && <>{badge}</>}
</IconContainer>
</NavItemContainer>
)
}
<NavIconItem icon="notification" badge={<RedDotBadge/>} tooltip='notification' isActive={true}/>
const BottomBarItem = ({ icon, isActive = false, children, onClick, href, ...rest }) => {
return (
<NavItemContainer data-active={isActive} onClick={onClick} href={href} {...rest}>
{isValidElement(icon) ? <>{icon}</> : <Icon icon={icon} />}
{children}
</NavItemContainer>
)
}
<BottomBarItem icon="notification" isActive={true}> {label} </BottomBarItem>
issue: @azkanurf-ss I think you may be confusing two different components and treating them as one. The "Nav Icon Item" is exclusively used for the Secondary Nav and only has a visible label when hovered or focused (i.e. the when the tooltip is displayed), while the screenshot you show of an icon + visible label is the "Bottom Bar Item" and is used exclusively for the Bottom Bar.
issue: As I've recommended for the Button work currently in-progress, we should avoid tightly coupling our components to the current Icon
component. Instead, we should simply accept a generic ReactNode for the icon. I would probably take the same approach with the badge as well.
suggestion: I know the App bar design specs only show the dot-style badge, but the Design System as a numerical style badge as well. It would be worth clarifying with Design whether secondary nav items should support the numerical style as well (I've seen designs for Console Cloud that have shown numerical badges).
issue: Nav items will need to support rendering out as both <a>
elements and <button>
elements. I don't see any comments above regarding how you'll handle that.
question: Tooltips and badges are both atoms in their own rights. Are you planning on implementing those components as part of this work?
issue: @azkanurf-ss I think you may be confusing two different components and treating them as one. The "Nav Icon Item" is exclusively used for the Secondary Nav and only has a visible label when hovered or focused (i.e. the when the tooltip is displayed), while the screenshot you show of an icon + visible label is the "Bottom Bar Item" and is used exclusively for the Bottom Bar.
Thank you for clarifying! You’re right. I was mixing up the two components. I’ll update my outline to separate them into NavIconItem and BottomBarItem.
issue: As I've recommended for the Button work currently in-progress, we should avoid tightly coupling our components to the current Icon component. Instead, we should simply accept a generic ReactNode for the icon. I would probably take the same approach with the badge as well.
issue: Nav items will need to support rendering out as both elements and
Sure, I’ve been following the Button work closely, and I agree. I’ll also adopt the same approach to support both href and onClick.
question: Tooltips and badges are both atoms in their own rights. Are you planning on implementing those components as part of this work?
I was considering creating a base component for NavIconItem and BottomBarItem, since the main difference between the two components is that NavIconItem will have tooltip and badge props, while BottomBarItem will have a label prop So in that case, I can create 3 PRs:
suggestion: I know the App bar design specs only show the dot-style badge, but the Design System as a numerical style badge as well. It would be worth clarifying with Design whether secondary nav items should support the numerical style as well (I've seen designs for Console Cloud that have shown numerical badges).
I’ll raise this query with the Design team to clarify whether secondary nav items should support the numerical style badge as well.
Hi @kurtdoherty, the Design team has responded to the query here. Do you think we still need to use the Icon approach for the Badge?
Edit: I've updated the outline, but I'm still uncertain about the names for <NavIconItem />
and <NavItemContainer />
.
I'm considering renaming:
<NavIconItem />
to <SecondaryNavItem />
<NavItemContainer />
to <NavIconItemContainer />
I was considering creating a base component for NavIconItem and BottomBarItem
I think we need to be careful creating dependencies between components that the Design team have kept separate. I don't think the cost of duplicating some styles etc will be too high for either of these components, so I'm personally okay to keep them completely separate. If you feel differently, it's worth raising with the Design team (if you haven't already).
the Design team has responded to the query here. Do you think we still need to use the Icon approach for the Badge?
Yes, I think that response confirms we probably want the flexibility to provide different badges in future, so I think accepting a ReactNode
prop for the badge is fine. Do we have a finalised design spec for the Badge component? If not, I think that's even more reason to just accept a ReactNode
here.
I'm still uncertain about the names for
and .
I'm not sure I understand the need for the NavItemContainer
... what do you envision it facilitating? Same with the IconContainer
component you show in the snippets above.
question: is the use of title
for the tooltip
a temporary decision? I ask because the native title
attribute does not produce a tooltip that we can style with CSS (at least to my knowledge), so I'm not sure how using that attribute allows us to produce the tooltip shown in the designs...
issue: re icon
, I don't think we should be doing {isValidElement(icon) ? <>{icon}</> : <Icon icon={icon} />}
. That still implicitly couples us to the existing Icon
component, which is what we're trying to avoid by accepting any ReactNode
in the first place.
The examples you show would instead look like:
<NavIconItem icon={<Icon name="notification" />} badge={<RedDotBadge/>} tooltip='notification' isActive={true}/>
<BottomBarItem icon={<Icon name="notification" />} isActive={true}>{label}</BottomBarItem>
I think we need to be careful creating dependencies between components that the Design team have kept separate. I don't think the cost of duplicating some styles etc will be too high for either of these components, so I'm personally okay to keep them completely separate.
I understand your point about keeping components separate. I think it is also easier to maintain if there's a major update in the future for that component. So, I’m on board with keeping them distinct.
I'm not sure I understand the need for the NavItemContainer... what do you envision it facilitating? Same with the IconContainer component you show in the snippets above.
We can ignore that for now since we plan to separate both components and will copy the pattern from the Button component. So, I’ll temporarily remove my proposed code section but keep the React usage code.
is the use of title for the tooltip a temporary decision?
Yes it is, it's just so we know we'll have tooltip prop. Sorry for any confusion🙏
Yes, I think that response confirms we probably want the flexibility to provide different badges in future, so I think accepting a ReactNode prop for the badge is fine. Do we have a finalised design spec for the Badge component? If not, I think that's even more reason to just accept a ReactNode here.
The design team has already provided the handoff file for the badge here, but I think we need a smaller badge size for the Nav Icon Item.
The design team has already provided the handoff file for the badge here, but I think we need a smaller badge size for the Nav Icon Item.
I think that's a completely different component 🤔 I think I've gotten confused about the component's name and I can't seem to find the component I thought did exist for this dot and a numerical version of it... I think we need to ask Design for some clarity.
Hi @kurtdoherty , I'll lend my back to carry on this ticket and thanks to @azkanurf-ss who already initiate the proposal for bottom bar item and secondary nav item
thought: agree to both, I think it would be best for us to scoping this ticket only to cover the NavIconItem component.
thought: Looks like we could start to initiate the basic of NavIconItem without worrying the extra ornaments such like Tooltip and the "Numerical Indicator" (which will replaced with passing ReactNode interface), for rough estimation this component could be used for React user like:
// anchor version (using anchor behind the scene)
<NavIconItem icon={<Icon name="notification" />} badge={<RedDotBadge/>} isActive href="https://foo" />
// callback version (using button behind the scene)
<NavIconItem icon={<Icon name="notification" />} badge={<RedDotBadge/>} isActive onClick={bar} />
@ss-dimasm no worries. I agree that work can start 👍
I think we just need to work with Design to find out what component the "dot" actually represents (if any) so that we can name the associated prop correctly. Since Badge
seems to be something completely different, I'm not sure that's an appropriate name for a prop on this NavIconItem
.
Following the discussion from #191 . we're still need to polish this component by adding the red dot indicator and also a tooltip; but before moving forward; I've raise a question to Design Team via Figma; we could track it in here
but before moving forward; I've raise a question to Design Team via Figma; we could track it in here
it depends on the answer, if they treat the Red Dot Indicator and Tooltip as a shared component, I think it's better to raise a new ticket for this; and continue to work the NavIconItem
after it's merged; but if not, looks like we could move straight forward to implement directly to the NavIconItem
Abstract
To support the AppBar improvements the NavIconButton and NavIconMenuButton components should be made which can be used in secondary and footer navigation
NavIconButton is a button with a centred icon and optional centred text below. It can be a private component used by AppBar
NavIconMenuButton is an extended version of NavIconButton which shows the menu above or below
Specification
Developer Checklist
Release Checklist
Additional Context or Information