-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: add Spacer component #248
Conversation
/** | ||
* Sizes: | ||
* - xxs: 5px | ||
* - xs: 10px | ||
* - s: 20px | ||
* - m: 30px | ||
* - l: 60px | ||
* - xl: 90px | ||
* - xxl: 180px | ||
*/ |
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.
src/spacer/spacer.tsx
Outdated
// NOTE: Do not construct class names dynamically | ||
// https://tailwindcss.com/docs/content-configuration#classes-arent-generated | ||
if (size === "xxs") { | ||
cls = "h-[5px]"; |
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.
The Spacer in /learn is a div with top and bottom paddings, and the padding values are just the padding on one side.
If we have:
<Spacer size="medium" />
It will be a div
with 15px of padding on the top and bottom, so the total height will be 30px.
This is too much math for me, so with the new component, I changed the height
value instead. I also went ahead and used t-shirt sizes for size
.
Name/value mapping:
/learn Spacer | fcc/ui Spacer |
---|---|
xxSmall - 2.5px |
xxs - 5px |
small - 5px |
xs - 10px |
xSmall - 10px |
s - 20px |
medium - 15px |
m - 30px |
large - 30px |
l - 60px |
exLarge - 45px |
xl - 90px |
doubleXL - 90px |
xxl - 180px |
|
||
type Size = "xxs" | "xs" | "s" | "m" | "l" | "xl" | "xxl"; | ||
|
||
interface SpacerProps { |
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.
The /learn Spacer accepts children, which was needed to account for a section on the profile page (freeCodeCamp/freeCodeCamp#49828). The rendering logic of the page has changed and I'm not seeing this kind of usage anywhere in /learn, so I'm not including this feature in the new version.
IMHO, I don't think Spacer should accept any children. As the name implies, it should just be a simple block or gap. Anything fancier should be handled by a layout component (#13).
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.
LGTM as is, but I think the code reads a bit better if we refactor the if-else chain:
Co-authored-by: Oliver Eyton-Williams <[email protected]>
Checklist:
Update index.md
)This PR adds a new Spacer component to the library.
This component will be a replacement for https://github.com/freeCodeCamp/freeCodeCamp/blob/main/client/src/components/helpers/spacer.tsx.
Screenshot