Add Timeline Component #35
|
@ -0,0 +1,75 @@
|
|||
.wrapper {
|
||||
position: relative;
|
||||
}
|
||||
j285he marked this conversation as resolved
Outdated
|
||||
|
||||
.line {
|
||||
j285he marked this conversation as resolved
Outdated
a258wang
commented
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
a258wang
commented
This doesn't need to be absolutely positioned. Try setting the z-index on 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
a258wang
commented
Is this necessary? 🤔 Is this necessary? 🤔
|
||||
flex-direction: row;
|
||||
justify-content: center;
|
||||
j285he marked this conversation as resolved
Outdated
a258wang
commented
Is 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
a258wang
commented
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
a258wang
commented
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 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 +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
a258wang
commented
Is this necessary? It looks like it's only really being used for the line, but we could just set 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;
|
||||
a258wang
commented
What happens if the timeWidth + textWidth is greater than the overall width? 👀 I see a few options with regards to customizing widths:
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.
j285he
commented
So I actually added three more variables 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
a258wang
commented
NIT: maybe rename to 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})`
|
||||
);
|
||||
a258wang
commented
Would The line also looks not perfectly centered on staging - it's very slightly to the right. I think this is because 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.
j285he
commented
Good idea regarding the issue with 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?
j285he
commented
If so, do we prefer something like
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
a258wang
commented
NIT: NIT: `<div />` instead of `<div></div>` :>
|
||||
|
||||
j285he marked this conversation as resolved
Outdated
a258wang
commented
Is the Is the `width` necessary here, since the CSS already specifies `width: 100%`?
|
||||
return (
|
||||
<div
|
||||
className={
|
||||
j285he marked this conversation as resolved
Outdated
a258wang
commented
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 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?
j285he
commented
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
a258wang
commented
NIT: get rid of extra empty line NIT: get rid of extra empty line
|
||||
}
|
||||
|
||||
interface TimelineSectionProps {
|
||||
datum: TimelineData;
|
||||
j285he marked this conversation as resolved
a258wang
commented
Have we considered using 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.)
j285he
commented
Ok, using css hover on 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.
a258wang
commented
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
a258wang
commented
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
a258wang
commented
These spacer divs should (hopefully?) no longer be necessary if we get rid of 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
a258wang
commented
can probably use 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
a258wang
commented
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
a258wang
commented
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
a258wang
commented
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>
|
||||
);
|
||||
}
|
|
@ -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",
|
||||
|
|
|
@ -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>
|
||||
|
|
Is
height: 100%
necessary here?