Skip to content
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

Feature/2 add badges #26

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Feature/2 add badges #26

wants to merge 3 commits into from

Conversation

Shoaibv2
Copy link
Member

Fixes #2

@Shoaibv2 Shoaibv2 requested a review from ranajahanzaib April 14, 2022 09:32
@Shoaibv2 Shoaibv2 changed the title Feature/2 add badges #26 Feature/2 add badges Apr 14, 2022
@ranajahanzaib
Copy link
Contributor

@aftabrehan please review.

Copy link
Member

@aftabrehan aftabrehan left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

Just a minor changes. Good work. 💯

Comment on lines 1 to 15
import { Badge, PillBadges, ButtonBadges } from "kbs-core";

function Badges() {
return (
<div>
<Badge variant="primary" label="Primary" />
<Badge variant="Seconday" label="Badge" />
<Badge variant="Dark h1" label="Badge" />
<PillBadges variant="primary" label="Primary" />
<ButtonBadges buttonvariant="primary" labelvariant="light" label="2" />
</div>
);
}

export default Badges;
Copy link
Member

Choose a reason for hiding this comment

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

Move this in badge.ts 🙏

Use first letter capital only for components name, and just write your code in .ts file. Thanks!

</span>
);
}
export default PillBadges;
Copy link
Member

@aftabrehan aftabrehan Apr 14, 2022

Choose a reason for hiding this comment

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

Rename this file as BadgePill.tsx. 👉

Copy link
Member

Choose a reason for hiding this comment

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

This makes much sense.

import React from "react";
import Badge from "./components/Badge";
import PillBadges from "./components/PillBadges";
import ButtonBadges from "./components/ButtonBadge";
export { Button, type ButtonProps } from "./Button";
Copy link
Member

Choose a reason for hiding this comment

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

It's a good practice to have an empty line between imports & exports. 👍

Suggested change
export { Button, type ButtonProps } from "./Button";
export { Button, type ButtonProps } from "./Button";

</button>
);
}
export default ButtonBadges;
Copy link
Member

Choose a reason for hiding this comment

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

Can be it renamed as BadgeButton.tsx.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@@ -0,0 +1,9 @@
function ButtonBadges(props: any) {
return (
<button type="button" className={`btn btn-${props.buttonvariant}`}>
Copy link
Member

Choose a reason for hiding this comment

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

Use camelCase.

Suggested change
<button type="button" className={`btn btn-${props.buttonvariant}`}>
<button type="button" className={`btn btn-${props.buttonVariant}`}>

return (
<button type="button" className={`btn btn-${props.buttonvariant}`}>
Notifications
<span className={`badge bg-${props.labelvariant}`}>{props.label}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Again...

Suggested change
<span className={`badge bg-${props.labelvariant}`}>{props.label}</span>
<span className={`badge bg-${props.labelVariant}`}>{props.label}</span>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Badges Component
3 participants