Add Timeline Component #35
Labels
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: www/cs-2022-class-profile#35
Loading…
Reference in New Issue
No description provided.
Delete Branch "j285he-timeline"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #7
Staging: https://j285he-timeline-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/
Yoooo the timeline actually looks so awesome, great work!!! Just left some nits on how the styling is implemented and other nitpicky details. :p
@ -0,0 +2,4 @@
position: relative;
}
.line {
The timeline line on Figma has a slightly rounded end at the bottom, could we try to replicate here as well?
@ -0,0 +9,4 @@
}
.timelineSections {
position: absolute;
This doesn't need to be absolutely positioned. Try setting the z-index on
.line
to be negative so it appears behind everything else.@ -0,0 +20,4 @@
.timelineSection {
width: 100%;
height: inherit;
Is this necessary? 🤔
@ -0,0 +40,4 @@
}
.circle {
margin-left: calc(2rem / 16);
What's the purpose of the left margin here?
@ -0,0 +72,4 @@
}
.textHover {
border: calc(2rem / 16) solid var(--secondary-accent-light);
I'm noticing a slight displacement when hovering over the timeline sections, which I think is being caused by this border being added. The easiest way to fix this would probably be to add a
--card-background
coloured or a--background
coloured border of the same thickness to the unhovered text card.@ -0,0 +12,4 @@
/** Width of the entire timeline, in pixels. */
width: number;
/** Height of the entire timeline, in pixels. */
height: number;
Is this necessary? It looks like it's only really being used for the line, but we could just set
height: 100%
on that and it'll fill the wrapper.My opinion is that no one is going to know what they want the height of the timeline to be, since it basically just depends on the data, so it feels a little awkward to make someone specify the height when using the timeline component.
@ -0,0 +18,4 @@
/** Width of time label, in pixels. */
timeWidth?: number;
/** Width of text label, in pixels. */
textWidth?: number;
What happens if the timeWidth + textWidth is greater than the overall width? 👀
I see a few options with regards to customizing widths:
justify-content: begin
orend
and then position the line by adding together the appropriate widths/offsets.)justify-content: end
for each timeline section, and then position the line withright: textWidth + labelsOffset
etc.)I personally like option 3, but I think they're all reasonable so I'll leave it up to you to choose.
So I actually added three more variables
outerCircleWidth
,innerCircleWidth
andlineWidth
to get rid of the constants that were present in the code.What do you think the specifications should be here?
There's an error check if all the elements' width added up is larger than the total width.
Could we just let the programmer fix the widths if they are too large and deem the thrown errors to be sufficient?
@ -0,0 +20,4 @@
/** Width of text label, in pixels. */
textWidth?: number;
/** Distance between labels to middle line, in pixels. */
labelsOffset?: number;
NIT: maybe rename to
gap
and/or clarify the jsdoc? When I read "label" I thought of the time label only, and didn't realize this prop also controlled the gap between the line and the text cards.@ -0,0 +43,4 @@
>
<div
className={styles.line}
style={{ height: height, left: width / 2 }}
Would
height: 100%
andleft: 50%
work?The line also looks not perfectly centered on staging - it's very slightly to the right. I think this is because
left
is the distance between the left edge of the timeline component and the left edge of the line, but we want width / 2 (ie. 50%) to be the distance between the left edge of the timeline component and the middle of the line, so we probably just have to add/subtract half the width of the line here.Good idea regarding the issue with
left
I decided to add a lineWidth prop to adjust the central timeline width because I think this might be useful in the future.
This means I will need to use an inline style with the prop, is this ok?
If so, do we prefer something like
@ -0,0 +44,4 @@
<div
className={styles.line}
style={{ height: height, left: width / 2 }}
></div>
NIT:
<div />
instead of<div></div>
:>@ -0,0 +45,4 @@
className={styles.line}
style={{ height: height, left: width / 2 }}
></div>
<div className={styles.timelineSections} style={{ width: width }}>
Is the
width
necessary here, since the CSS already specifieswidth: 100%
?@ -0,0 +76,4 @@
width,
isTimeUppercase,
timeWidth,
NIT: get rid of extra empty line
@ -0,0 +80,4 @@
textWidth,
labelsOffset,
}: TimelineSectionProps) {
const [onHover, setHover] = useState(false);
Have we considered using
.timelineSection:hover time
,.timelineSection:hover text
, etc. CSS selectors to handle the hover styles?The current hovering works fine, my only (minor) qualm is that it feels a bit odd that currently the hover styles are triggered by mousing over the space underneath the time label, but not by mousing over the gaps. (Also currently if the user moves their mouse quickly in a horizontal direction across the timeline, the section will quickly switch between hovered/unhovered styles as the cursor moves between time - gap - circle - gap - text, etc.)
Ok, using css hover on
.timelineSection
will still result in hover effects triggering for the space underneath labels or in the space between the labels and the edge of the timeline.Okay, I think that makes sense (in my opinion) since now it's like all the gaps/empty spaces trigger the hover effect, whereas previously the space underneath the time would trigger the hover effects but other spaces/gaps wouldn't.
@ -0,0 +84,4 @@
const handleMouseEnter = () => {
setHover(true);
console.log(onHover);
NIT: get rid of debug statements before we ship to prod 😁
@ -0,0 +93,4 @@
<div className={styles.timelineSection}>
<div
style={{
width: (width - labelsOffset - labelsOffset - 30) / 2 - timeWidth,
These spacer divs should (hopefully?) no longer be necessary if we get rid of
position: absolute
on.timelineSection
and can be replaced withmargin
/padding
andgap
. 🙂@ -0,0 +106,4 @@
>
{isTimeUppercase ? datum.time.toUpperCase() : datum.time}
</div>
<div style={{ width: labelsOffset }}></div>
can probably use
style={{ gap: labelsOffset }}
on the timeline section div, once we fix the flexbox-y and absolute positioning shenanigans@ -0,0 +109,4 @@
<div style={{ width: labelsOffset }}></div>
<div
className={styles.circle}
style={{ width: 30 }}
Is this necessary if it's already specified in the CSS file?
@ -0,0 +115,4 @@
>
<div
className={styles.innerCircle}
style={{ display: onHover ? "inline" : "none" }}
NIT: we generally prefer to style using CSS files instead of inline. :)
@a258wang requesting re-review
Add timeline componentto Add Timeline componentAdd Timeline componentto Add Timeline ComponentSorry, just a few more nitpicks/questions!
@ -0,0 +1,78 @@
.wrapper {
position: relative;
height: 100%;
Is
height: 100%
necessary here?@ -0,0 +22,4 @@
.timelineSection {
width: 100%;
height: 100%;
Is
height: 100%
necessary here?@ -0,0 +48,4 @@
throw new Error(
`<Timeline /> - timeWidth + gap + ${
outerCircleWidth > lineWidth ? "outerCircleWidth" : "lineWidth"
} + gap + textWidth (${timeWidth} + ${gap} + ${largerMiddleElemeent} + ${gap} + ${textWidth} = ${requestedWidth}) is larger than width (${width})`
I think this error-checking is a good idea overall, however I'm still not convinced that there's a use to being able to specify
width
as a prop when the effective width of the component can be calculated astimeWidth + gap + largerMiddleElemeent + gap + textWidth
. In my opinion, it's sufficient to havetimeWidth
,gap
,textWidth
, etc. as props, then if the dev really wants the entire component to have a specific width, they can either make sure that the relevant props add up to the number they want, or use a className to add margin. Is there something I'm missing?Ahh that makes sense!
@ -0,0 +111,4 @@
textWidth,
gap,
}: TimelineSectionProps) {
const [onHover, setHover] = useState(false);
Clean up the hover state and onMouseEnter/Leave shenanigans, if we're going to do hovering in CSS :))
🚢