-
Notifications
You must be signed in to change notification settings - Fork 2
Issue 16 create feature box component #19
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
base: main
Are you sure you want to change the base?
Conversation
Created the component as FeatureBox with two props: width and height, so you can dynamically change its size
|
would you please be able to include a screenshot of the component in the PR description? thanks! |
|
Done, tell me if you need any changes |
SafetyInObscurity
left a comment
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.
A good start! Some minor details to iron out before the pull
Added a custom dark purple colour to the config and globals files, seen in the desired component in the issue brief, as well as moving Also revised the feature box's props to title and text, instead of height and width (which now change from rem), which are strings that will be placed in the respective parts of the feature box.
SafetyInObscurity
left a comment
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.
Looking good! A few steps left for mobile compatibility and simplicity.
| <div | ||
| style={{ | ||
| width: "50rem", | ||
| height: "13rem", |
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.
I think the issue with the mobile version is this use of a fixed width. Ideally we want the element to fill the space it's given, rather than a fixed value. This also allows it to be used when it's not expected to fill the full viewport width.
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.
Setting the width relative to the viewport width prevents the element from being used when the parent element doesn't take the full viewport.
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.
I wanted to use percentages but the parent element might not be the root of the page? Is that what you recommend anyway or should I dynamically base it off of the width of a specific element?
-Added a third prop called hOffset, which simply changes the marginLeft of the component, allowing for horizontal freedom
-Changed the geometry of the shape, where the trapezoids on the top-left and bottom-right corners have a fixed height, and so don't change as the box squishes inwards
- Made the component's width responsive at 60vw as opposed to a fixed rem value
- Made the component's height responsive to the amount of content, e.g it's height increases as its width decreases to still contain all the text
- Replaced divs containg title and text with appropriate h3 and p
- Migrated most properties in style to tailwind classes, some calc() and ${} things don't work in it so have been kept in style
Update dark-alt color value in globals.css
SafetyInObscurity
left a comment
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.
Looking good! If you're having trouble with proportions, remember box-border enables padding and borders in calculations of percentage dimensions.
-Removed the set width for the component at 60vw, the component will simply have a maximum width of 60% of the parent element's width. -Changed some style properties of the component, notably changing the background of the content of the box to be the standardised dark-alt colour -Removed the hOffset component as it is assumed to be horizontally centered -Modified the boxSizing property of the component to 'border-box', which will hopefully help more with responsiveness
Change Summary
Created a component named FeatureBox that fulfills the description of this issue. Along with the special shape, it's title and text contents can be modified per component via the title and text props. Its width and height are responsive to the amount of text, scaling only to fit the space that it needs, with the prop's max width being 60% of its parent elements
featureBox.mp4
Change Form
Other Information
Related issue