-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add Roadmap table #2051
Add Roadmap table #2051
Conversation
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.
Left you some suggestions for this PR, but also leaving an approval to not block you.
|
||
function parseCustomDate(dateStr: string): Date { | ||
if (dateStr.length <= 0) { | ||
return new Date(2999, 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.
Curious what this is for?
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 readability, the dates are presented as "Oct '22", "Nov '23", etc. The default sorting function handles these alphabetically, which leads to an incorrect order, so this function makes it behave as expected.
In a future rewrite I'd probably store the dates as true Dates and then render them in the desired format, but for now I'll leave it as-is.
aValue = parseCustomDate(aValue); | ||
bValue = parseCustomDate(bValue); | ||
|
||
console.log(aValue, bValue); |
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.
Need this?
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.
Nope, good catch!
const severity = | ||
value === "Fully released" | ||
? "success" | ||
: value === "In Labs" | ||
? "warning" | ||
: value === "In progress" | ||
? "default" | ||
: value === "Not started" | ||
? "error" | ||
: "default"; |
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.
This would be better as a switch block.
Cell: ({ cell }) => { | ||
return cell.getValue() ? cell.getValue() : "—"; | ||
}, |
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.
A lot of repeated code below... perhaps extract this function out to a variable for reuse?
As discussed with Hannah, we've decided the best way to provide a Roadmap table with all the needed features is to create it using our own
DataTable
component and then embed it in Supernova.This PR adds a Roadmap page to the Storybook sidebar, and outputs a table that can be embedded in Supernova too.
Todo:
@okta/odyssey-design-tokens
and@storybook/theming
out ofdevDependencies
and intodependencies
in the Storybook package.json