Add Timeline Component #35

Merged
j285he merged 17 commits from j285he-timeline into main 3 months ago
Collaborator
Closes #7 Staging: https://j285he-timeline-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/
j285he added 7 commits 3 months ago
02846d9459 Initial component
f0d3a97107 Merge branch 'main' into j285he-timeline
c1b8ae469f Redo everything
59b476cd5e CSS cleanup
j285he requested review from a258wang 3 months ago
j285he requested review from snedadah 3 months ago
j285he requested review from e26chiu 3 months ago
j285he requested review from b72zhou 3 months ago
j285he requested review from n3parikh 3 months ago
j285he added 1 commit 3 months ago
5f86a6d74c npm uninstall @visx/annotation
j285he added 1 commit 3 months ago
3de0f67ccb px -> rem
a258wang reviewed 3 months ago
a258wang left a comment
Owner

Yoooo the timeline actually looks so awesome, great work!!! Just left some nits on how the styling is implemented and other nitpicky details. :p

Yoooo the timeline actually looks so awesome, great work!!! Just left some nits on how the styling is implemented and other nitpicky details. :p
position: relative;
}
.line {
Owner

The timeline line on Figma has a slightly rounded end at the bottom, could we try to replicate here as well?

The timeline line on Figma has a slightly rounded end at the bottom, could we try to replicate here as well?
j285he marked this conversation as resolved
}
.timelineSections {
position: absolute;
Owner

This doesn't need to be absolutely positioned. Try setting the z-index on .line to be negative so it appears behind everything else.

This doesn't need to be absolutely positioned. Try setting the z-index on `.line` to be negative so it appears behind everything else.
j285he marked this conversation as resolved
.timelineSection {
width: 100%;
height: inherit;
Owner

Is this necessary? 🤔

Is this necessary? 🤔
j285he marked this conversation as resolved
}
.circle {
margin-left: calc(2rem / 16);
Owner

What's the purpose of the left margin here?

What's the purpose of the left margin here?
j285he marked this conversation as resolved
}
.textHover {
border: calc(2rem / 16) solid var(--secondary-accent-light);
Owner

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.

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.
j285he marked this conversation as resolved
/** Width of the entire timeline, in pixels. */
width: number;
/** Height of the entire timeline, in pixels. */
height: number;
Owner

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.

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.
j285he marked this conversation as resolved
/** Width of time label, in pixels. */
timeWidth?: number;
/** Width of text label, in pixels. */
textWidth?: number;
Owner

What happens if the timeWidth + textWidth is greater than the overall width? 👀

I see a few options with regards to customizing widths:

  1. Only customize the width of the entire timeline (and the labelsOffset), and always have the line running straight down the middle, with the text card taking up all the space it has available.
  2. Only customize the width of the time label and text card (and the labelsOffset), and don't customize the overall timeline width as it'll adapt based on the other values. (In this case I'd suggest trying justify-content: begin or end and then position the line by adding together the appropriate widths/offsets.)
  3. Customize the width of the entire timeline and the text card (and the labelsOffset), then throw an error or return null or something if the text card width exceeds the timeline width. The time label can just take up all the space it has available. (In this case I'd suggest trying justify-content: end for each timeline section, and then position the line with right: 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.

What happens if the timeWidth + textWidth is greater than the overall width? 👀 I see a few options with regards to customizing widths: 1. Only customize the width of the entire timeline (and the labelsOffset), and always have the line running straight down the middle, with the text card taking up all the space it has available. 2. Only customize the width of the time label and text card (and the labelsOffset), and don't customize the overall timeline width as it'll adapt based on the other values. (In this case I'd suggest trying `justify-content: begin` or `end` and then position the line by adding together the appropriate widths/offsets.) 3. Customize the width of the entire timeline and the text card (and the labelsOffset), then throw an error or return null or something if the text card width exceeds the timeline width. The time label can just take up all the space it has available. (In this case I'd suggest trying `justify-content: end` for each timeline section, and then position the line with `right: 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.
Poster
Collaborator

So I actually added three more variables outerCircleWidth, innerCircleWidth and lineWidth 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?

So I actually added three more variables `outerCircleWidth`, `innerCircleWidth` and `lineWidth` 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?
/** Width of text label, in pixels. */
textWidth?: number;
/** Distance between labels to middle line, in pixels. */
labelsOffset?: number;
Owner

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.

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.
j285he marked this conversation as resolved
>
<div
className={styles.line}
style={{ height: height, left: width / 2 }}
Owner

Would height: 100% and left: 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.

Would `height: 100%` and `left: 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.
Poster
Collaborator

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?

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?
Poster
Collaborator

If so, do we prefer something like

left: `calc(50% - ${lineWidth}px / 2)`


or



left: width / 2 - lineWidth / 2
If so, do we prefer something like ``` left: `calc(50% - ${lineWidth}px / 2)` or left: width / 2 - lineWidth / 2 ```
<div
className={styles.line}
style={{ height: height, left: width / 2 }}
></div>
Owner

NIT: <div /> instead of <div></div> :>

NIT: `<div />` instead of `<div></div>` :>
j285he marked this conversation as resolved
className={styles.line}
style={{ height: height, left: width / 2 }}
></div>
<div className={styles.timelineSections} style={{ width: width }}>
Owner

Is the width necessary here, since the CSS already specifies width: 100%?

Is the `width` necessary here, since the CSS already specifies `width: 100%`?
j285he marked this conversation as resolved
width,
isTimeUppercase,
timeWidth,
Owner

NIT: get rid of extra empty line

NIT: get rid of extra empty line
j285he marked this conversation as resolved
textWidth,
labelsOffset,
}: TimelineSectionProps) {
const [onHover, setHover] = useState(false);
Owner

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.)

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.)
Poster
Collaborator

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.

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.
Owner

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.

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.
j285he marked this conversation as resolved
const handleMouseEnter = () => {
setHover(true);
console.log(onHover);
Owner

NIT: get rid of debug statements before we ship to prod 😁

NIT: get rid of debug statements before we ship to prod 😁
j285he marked this conversation as resolved
<div className={styles.timelineSection}>
<div
style={{
width: (width - labelsOffset - labelsOffset - 30) / 2 - timeWidth,
Owner

These spacer divs should (hopefully?) no longer be necessary if we get rid of position: absolute on .timelineSection and can be replaced with margin/padding and gap. 🙂

These spacer divs should (hopefully?) no longer be necessary if we get rid of `position: absolute` on `.timelineSection` and can be replaced with `margin`/`padding` and `gap`. 🙂
j285he marked this conversation as resolved
>
{isTimeUppercase ? datum.time.toUpperCase() : datum.time}
</div>
<div style={{ width: labelsOffset }}></div>
Owner

can probably use style={{ gap: labelsOffset }} on the timeline section div, once we fix the flexbox-y and absolute positioning shenanigans

can probably use `style={{ gap: labelsOffset }}` on the timeline section div, once we fix the flexbox-y and absolute positioning shenanigans
j285he marked this conversation as resolved
<div style={{ width: labelsOffset }}></div>
<div
className={styles.circle}
style={{ width: 30 }}
Owner

Is this necessary if it's already specified in the CSS file?

Is this necessary if it's already specified in the CSS file?
j285he marked this conversation as resolved
>
<div
className={styles.innerCircle}
style={{ display: onHover ? "inline" : "none" }}
Owner

NIT: we generally prefer to style using CSS files instead of inline. :)

NIT: we generally prefer to style using CSS files instead of inline. :)
j285he marked this conversation as resolved
j285he added 1 commit 3 months ago
4f6f3265fd Code review fixes
Poster
Collaborator

@a258wang requesting re-review

@a258wang requesting re-review
j285he changed title from Add timeline component to Add Timeline component 3 months ago
j285he changed title from Add Timeline component to Add Timeline Component 3 months ago
a258wang reviewed 3 months ago
a258wang left a comment
Owner

Sorry, just a few more nitpicks/questions!

Sorry, just a few more nitpicks/questions!
.wrapper {
position: relative;
height: 100%;
Owner

Is height: 100% necessary here?

Is `height: 100%` necessary here?
j285he marked this conversation as resolved
.timelineSection {
width: 100%;
height: 100%;
Owner

Is height: 100% necessary here?

Is `height: 100%` necessary here?
j285he marked this conversation as resolved
throw new Error(
`<Timeline /> - timeWidth + gap + ${
outerCircleWidth > lineWidth ? "outerCircleWidth" : "lineWidth"
} + gap + textWidth (${timeWidth} + ${gap} + ${largerMiddleElemeent} + ${gap} + ${textWidth} = ${requestedWidth}) is larger than width (${width})`
Owner

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 as timeWidth + gap + largerMiddleElemeent + gap + textWidth. In my opinion, it's sufficient to have timeWidth, 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?

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 as `timeWidth + gap + largerMiddleElemeent + gap + textWidth`. In my opinion, it's sufficient to have `timeWidth`, `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?
Poster
Collaborator

Ahh that makes sense!

Ahh that makes sense!
j285he marked this conversation as resolved
textWidth,
gap,
}: TimelineSectionProps) {
const [onHover, setHover] = useState(false);
Owner

Clean up the hover state and onMouseEnter/Leave shenanigans, if we're going to do hovering in CSS :))

Clean up the hover state and onMouseEnter/Leave shenanigans, if we're going to do hovering in CSS :))
j285he marked this conversation as resolved
j285he added 2 commits 3 months ago
4b3829894a Code review fixes
j285he added 1 commit 3 months ago
dd09329e62 Merge main
j285he added 1 commit 3 months ago
b529a24587 Do not make Timeline default export
j285he added 1 commit 3 months ago
039a9fc77c Merge main
j285he added 1 commit 3 months ago
3d329611eb Merge main
a258wang approved these changes 3 months ago
a258wang left a comment
Owner

🚢

🚢
j285he added 1 commit 3 months ago
36120123d8 Merge branch 'main' into j285he-timeline
j285he merged commit e3948c0577 into main 3 months ago
j285he deleted branch j285he-timeline 3 months ago
j285he referenced this issue from a commit 3 months ago

Reviewers

snedadah was requested for review 3 months ago
e26chiu was requested for review 3 months ago
b72zhou was requested for review 3 months ago
n3parikh was requested for review 3 months ago
a258wang approved these changes 3 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as e3948c0577.
Sign in to join this conversation.
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…
There is no content yet.