-
-
Notifications
You must be signed in to change notification settings - Fork 740
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
feat(1-3262): initial impl of new month/range picker #9122
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
export type Period = { | ||
key: string; | ||
dayCount: number; | ||
label: string; | ||
year: number; | ||
month: number; | ||
selectable: boolean; | ||
shortLabel: string; | ||
}; | ||
|
||
export const toSelectablePeriod = ( | ||
date: Date, | ||
label?: string, | ||
selectable = true, | ||
): Period => { | ||
const year = date.getFullYear(); | ||
const month = date.getMonth(); | ||
const period = `${year}-${(month + 1).toString().padStart(2, '0')}`; | ||
const dayCount = new Date(year, month + 1, 0).getDate(); | ||
return { | ||
key: period, | ||
year, | ||
month, | ||
dayCount, | ||
shortLabel: date.toLocaleString('en-US', { | ||
month: 'short', | ||
}), | ||
label: | ||
label || | ||
date.toLocaleString('en-US', { month: 'long', year: 'numeric' }), | ||
selectable, | ||
}; | ||
}; | ||
|
||
const currentDate = new Date(Date.now()); | ||
const currentPeriod = toSelectablePeriod(currentDate, 'Current month'); | ||
|
||
const getSelectablePeriods = (): Period[] => { | ||
const selectablePeriods = [currentPeriod]; | ||
for ( | ||
let subtractMonthCount = 1; | ||
subtractMonthCount < 12; | ||
subtractMonthCount++ | ||
) { | ||
// JavaScript wraps around the year, so we don't need to handle that. | ||
const date = new Date( | ||
currentDate.getFullYear(), | ||
currentDate.getMonth() - subtractMonthCount, | ||
1, | ||
); | ||
selectablePeriods.push( | ||
toSelectablePeriod(date, undefined, date > new Date('2024-03-31')), | ||
); | ||
} | ||
return selectablePeriods; | ||
}; |
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.
These functions are mostly copied from frontend/src/hooks/useTrafficData.ts
, but slightly modified. We'll probably want to reconsider this as the API is built out. I don't want to touch them more than I need to right now.
@@ -48,8 +48,7 @@ const calculateTrafficDataCost = ( | |||
return unitCount * trafficUnitCost; | |||
}; | |||
|
|||
const padMonth = (month: number): string => | |||
month < 10 ? `0${month}` : `${month}`; | |||
const padMonth = (month: number): string => month.toString().padStart(2, '0'); |
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.
Javascript can do this for us with a builtin instead of us checking the value
import { styled } from '@mui/material'; | ||
import { type FC, useState } from 'react'; | ||
|
||
export type Period = { |
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.
For me DateRange
is more intuitive, and easier to find in the code.
for ( | ||
let subtractMonthCount = 1; | ||
subtractMonthCount < 12; | ||
subtractMonthCount++ | ||
) { |
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.
Seeing months in reverse order looks a bit off. I'm used to pickers having same natural order, and maybe even starting line aligned to January
button: { | ||
cursor: 'pointer', | ||
border: 'none', | ||
background: 'none', | ||
fontSize: theme.typography.body1.fontSize, | ||
padding: theme.spacing(0.5), | ||
borderRadius: theme.shape.borderRadius, | ||
|
||
'&.selected': { | ||
backgroundColor: theme.palette.secondary.light, | ||
}, | ||
}, |
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.
For this to be consistent with other parts of the codebase, it should be done with props on a component (like <StyledButton selected={...}>
).
'li + li': { | ||
marginTop: theme.spacing(1), | ||
}, |
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.
flex
+ gap
can do the same, and be easier to understand, modify or debug.
className={ | ||
!tempOverride && | ||
period.key === selectedPeriod | ||
? 'selected' | ||
: '' | ||
} |
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.
Doing this with classes + nested selector instead of props makes it harder to refactor. There a link between how this element looks like, and where it is placed.
This PR implements a first version of the new month/range picker for the data usage graphs. It's minimally hooked up to the existing functionality to not take anything away.
This primary purpose of this PR is to get the design and interaction out on sandbox so that UX can have a look and we can make adjustments.
As such, there are a few things in the code that we'll want to clean up before removing the flag later:
NewHeader
component because the existing setup smushed the selector (it's a MUI grid setup, which isn't very flexible). I don't know what we want to do with this in the end, but the existing chart does have some problems when you resize your window, at least (although this is likely due to the chart, and can be solved in the same way that we did for the personal dashboards).