-
Notifications
You must be signed in to change notification settings - Fork 886
feat(site): add template insights page #8722
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
Conversation
@BrunoQuaresma could you please post some screenshots? |
@mtojek I posted a video! |
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.
Looks good to me 👍 I played a bit with this, and seems to work correctly.
Could you please write in the PR description what are the next steps? I assume that this is the MVP.
@@ -1862,7 +1865,9 @@ const ( | |||
// users to opt-in to via --experimental='*'. | |||
// Experiments that are not ready for consumption by all users should | |||
// not be included here and will be essentially hidden. | |||
var ExperimentsAll = Experiments{} | |||
var ExperimentsAll = Experiments{ | |||
ExperimentTemplateInsightsPage, |
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.
Is it intended to release as experimental?
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.
Yes, I talked with @bpmct about this
@@ -1,37 +0,0 @@ | |||
import { render } from "testHelpers/renderHelpers" |
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.
Is this deletion intended?
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.
Yeap, it is just a render test which should be on the storybook.
} | ||
} | ||
|
||
function sevenDaysAgo() { |
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.
nit: weekAgo :)
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 is a good one. I don't think a "week ago" is a good name for this because you are at Wed and a week ago would be the previous week from Mon to Fri so 7 days ago is different and for this case would be more accurate
@@ -3506,6 +3506,11 @@ | |||
"@types/connect" "*" | |||
"@types/node" "*" | |||
|
|||
"@types/chroma-js@2.4.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.
Is this change intended?
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.
yes, it is!
Close #8689
Demo:
Screen.Recording.2023-07-25.at.13.49.09.mov
Next steps: