Add Timeline Component #35

Merged
j285he merged 17 commits from j285he-timeline into main 2022-09-12 20:00:02 -04:00
4 changed files with 257 additions and 1 deletions

View File

@ -0,0 +1,75 @@
.wrapper {
position: relative;
}
j285he marked this conversation as resolved Outdated

Is height: 100% necessary here?

Is `height: 100%` necessary here?
.line {
j285he marked this conversation as resolved Outdated

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?
position: absolute;
height: 100%;
border-radius: calc(10rem / 16);
background-color: var(--secondary-accent);
z-index: -1;
}
j285he marked this conversation as resolved Outdated

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.
.timelineSections {
width: 100%;
display: flex;
flex-direction: column;
justify-content: space-around;
gap: calc(20rem / 16);
}
.timelineSection {
width: 100%;
display: flex;
j285he marked this conversation as resolved
Review

Is this necessary? 🤔

Is this necessary? 🤔
flex-direction: row;
justify-content: center;
j285he marked this conversation as resolved Outdated

Is height: 100% necessary here?

Is `height: 100%` necessary here?
}
.time {
margin: 0;
text-align: right;
font-size: calc(30rem / 16);
font-weight: 700;
color: var(--secondary-accent);
word-wrap: break-word;
}
.circle {
background-color: var(--secondary-accent);
box-shadow: calc(0rem / 16) calc(0rem / 16) calc(30rem / 16) var(--secondary-accent);
display: flex;
justify-content: center;
align-items: center;
}
j285he marked this conversation as resolved Outdated

What's the purpose of the left margin here?

What's the purpose of the left margin here?
.innerCircle {
background-color: var(--label);
display: none;
}
.text {
height: fit-content;
margin: 0;
padding: calc(15rem / 16);
border-radius: calc(10rem / 16);
font-size: calc(20rem / 16);
font-weight: 700;
color: var(--label);
border: calc(2rem / 16) solid var(--card-background);
background-color: var(--card-background);
word-wrap: break-word;
box-sizing: border-box;
}
.timelineSection:hover .time {
color: var(--secondary-accent-light);
}
.timelineSection:hover .innerCircle {
display: inline;
}
.timelineSection:hover .text {
border: calc(2rem / 16) solid var(--secondary-accent-light);
box-shadow: calc(0rem / 16) calc(0rem / 16) calc(20rem / 16) var(--secondary-accent);
}
j285he marked this conversation as resolved Outdated

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.

142
components/Timeline.tsx Normal file
View File

@ -0,0 +1,142 @@
import React from "react";
import styles from "./Timeline.module.css";
interface TimelineData {
time: string;
text: string;
}
interface TimelineProps {
data: TimelineData[];
/** Whether the time is transformed to uppercase. */
isTimeUppercase?: boolean;
/** Width of the middle timeline line, in pixels */
lineWidth?: number;
j285he marked this conversation as resolved Outdated

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.
/** Width of the outer circles on the timeline, in pixels. */
outerCircleWidth?: number;
/** Width of the inner circles on the timeline, in pixels. */
innerCircleWidth?: number;
/** Width of time label, in pixels. */
timeWidth?: number;

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.

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;
j285he marked this conversation as resolved Outdated

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.
/** Distance between the time label AND the text label to middle line, in pixels. */
gap?: number;
className?: string;
}
export function Timeline({
data,
isTimeUppercase = true,
lineWidth = 5,
outerCircleWidth = 30,
innerCircleWidth = 15,
timeWidth = 200,
textWidth = 300,
gap = 50,
className,
}: TimelineProps) {
const largerMiddleElement =
outerCircleWidth > lineWidth ? outerCircleWidth : lineWidth;
const width = timeWidth + gap + largerMiddleElement + gap + textWidth;
if (innerCircleWidth > outerCircleWidth) {
throw new Error(
`<Timeline /> - innerCircleWidth (${innerCircleWidth}) is larger than outerCircleWidth (${outerCircleWidth})`
);

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.

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?

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 ```
}
j285he marked this conversation as resolved Outdated

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

NIT: `<div />` instead of `<div></div>` :>
j285he marked this conversation as resolved Outdated

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

Is the `width` necessary here, since the CSS already specifies `width: 100%`?
return (
<div
className={
j285he marked this conversation as resolved Outdated

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?

Ahh that makes sense!

Ahh that makes sense!
className ? `${className} ${styles.wrapper}` : `${styles.wrapper}`
}
style={{ width: width }}
>
<div
className={styles.line}
style={{
width: lineWidth,
left: width / 2 - lineWidth / 2,
}}
/>
<div className={styles.timelineSections}>
{data.map((datum) => (
<TimelineSection
key={datum.time}
datum={datum}
width={width}
isTimeUppercase={isTimeUppercase}
outerCircleWidth={outerCircleWidth}
innerCircleWidth={innerCircleWidth}
timeWidth={timeWidth}
textWidth={textWidth}
gap={gap}
/>
))}
</div>
</div>
);
j285he marked this conversation as resolved Outdated

NIT: get rid of extra empty line

NIT: get rid of extra empty line
}
interface TimelineSectionProps {
datum: TimelineData;
j285he marked this conversation as resolved
Review

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

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

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.
width: number;
isTimeUppercase: boolean;
outerCircleWidth: number;
innerCircleWidth: number;
j285he marked this conversation as resolved Outdated

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

NIT: get rid of debug statements before we ship to prod 😁
timeWidth: number;
textWidth: number;
gap: number;
}
function TimelineSection({
datum,
width,
isTimeUppercase,
j285he marked this conversation as resolved
Review

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`. 🙂
outerCircleWidth,
innerCircleWidth,
timeWidth,
textWidth,
gap,
}: TimelineSectionProps) {
return (
<div className={styles.timelineSection} style={{ gap: gap }}>
<div
className={styles.time}
style={{
width: timeWidth,
marginLeft: (width - 2 * gap - outerCircleWidth) / 2 - timeWidth,
j285he marked this conversation as resolved Outdated

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
}}
>
{isTimeUppercase ? datum.time.toUpperCase() : datum.time}
j285he marked this conversation as resolved Outdated

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

Is this necessary if it's already specified in the CSS file?
</div>
<div
j285he marked this conversation as resolved
Review

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 :))
className={styles.circle}
style={{
width: outerCircleWidth,
height: outerCircleWidth,
j285he marked this conversation as resolved Outdated

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

NIT: we generally prefer to style using CSS files instead of inline. :)
borderRadius: outerCircleWidth,
}}
>
<div
className={styles.innerCircle}
style={{
width: innerCircleWidth,
height: innerCircleWidth,
borderRadius: innerCircleWidth,
}}
/>
</div>
<div
className={styles.text}
style={{
width: textWidth,
marginRight: (width - 2 * gap - outerCircleWidth) / 2 - textWidth,
}}
>
{datum.text}
</div>
</div>
);
}

View File

@ -80,6 +80,33 @@ export const moreMockCategoricalData = [
{ key: "Dart", value: 2.21 },
];
export const mockTimelineData = [
{
time: "Fall 2020",
text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.",
},
{
time: "Winter 2021",
text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad",
},
{
time: "Spring 2021",
text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor i",
},
{
time: "Fall 2021",
text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proid",
},
{
time: "Winter 2022",
text: "Lorem ipsum dolor sit amet, consectetur adipi",
},
{
time: "Spring 2022",
text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut en",
},
];
export const mockBoxPlotData = [
{
category: "1A",

View File

@ -7,12 +7,14 @@ import {
mockQuoteData,
mockQuoteDataLong,
mockPieData,
mockTimelineData,
} from "data/mocks";
import React from "react";
import About from "@/components/About";
import { PieChart } from "@/components/PieChart";
import { QuotationCarousel } from "@/components/QuotationCarousel";
import { Timeline } from "@/components/Timeline";
import { CenterWrapper } from "../components/CenterWrapper";
import { ColorPalette } from "../components/ColorPalette";
@ -25,13 +27,15 @@ export default function Home() {
<div className={styles.page}>
<h1>Playground</h1>
<p>Show off your components here!</p>
<ColorPalette />
<h2>
<code>{"<PieChart />"}</code>
</h2>
<div style={{ padding: "30px" }}>
<PieChart data={mockPieData} width={800} labelWidth={215} />
</div>
<ColorPalette />
<h2>
<code>Text Styles</code>
</h2>
@ -57,6 +61,7 @@ export default function Home() {
right: 20,
}}
/>
<h2>
<code>{"<BarGraphVertical />"}</code>
</h2>
@ -76,6 +81,7 @@ export default function Home() {
right: 20,
}}
/>
<h2>
<code>{"<WordCloud />"}</code>
</h2>
@ -85,6 +91,12 @@ export default function Home() {
value: word.value,
}))}
/>
<h2>
<code>{"<Timeline />"}</code>
</h2>
<Timeline data={mockTimelineData} />
<h2>
<code>{"<Textbox />"}</code>
</h2>