Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Bounty feat: Add NotificationDot component #208

Merged
merged 2 commits into from
Mar 9, 2021
Merged

Bounty feat: Add NotificationDot component #208

merged 2 commits into from
Mar 9, 2021

Conversation

z3s
Copy link
Contributor

@z3s z3s commented Mar 5, 2021

Support NotificationDot for Button, MenuItem, ...

Copy link
Contributor

@hachiojidev hachiojidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, I like this idea of a generic wrapper.


const NotificationDot: React.FC<NotificationDotProps> = ({ invisible = false, children, ...props }) => (
<NotificationDotRoot>
{Children.map(children, (child: ReactElement) => cloneElement(child, props))}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for cloning?

Copy link
Contributor Author

@z3s z3s Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ButtonMenu sets props to it's direct children, so NotificatonDot will receive MenuItem's props. then I use clone to pass the props to MenuItems.
https://github.com/pancakeswap/pancake-uikit/blob/master/src/components/ButtonMenu/ButtonMenu.tsx#L14-L23

background-color: ${({ theme }) => theme.colors.failure};
`;

const NotificationDot: React.FC<NotificationDotProps> = ({ invisible = false, children, ...props }) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we flip the invisible prop? Instead of negative assertion can we use a positive one. By default it should not show so the prop name should be something like show.

@RabbitDoge
Copy link
Contributor

If the requirement is just to add this dot on the profile icon, there is not need for a separated component, a pseudo class ::after is enough
cc @hachiojidev @Chef-Chungus

@Chef-Chungus
Copy link
Contributor

Chef-Chungus commented Mar 8, 2021

If the requirement is just to add this dot on the profile icon, there is not need for a separated component, a pseudo class ::after is enough
cc @hachiojidev @Chef-Chungus

@RabbitDoge it will be added on buttons and menu items
eg;
image

@z3s z3s requested a review from hachiojidev March 8, 2021 08:07
@hachiojidev hachiojidev merged commit 2069e75 into pancakeswap:master Mar 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants