-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Agency Dashboard] Dashboard UI scaffolding 1/n #146
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.
@terryttsai exciting to see this! Can you add some screenshots and/or videos to the PR description? Makes it a lot easier for me to review when I can cross reference the code with something visual
@@ -133,6 +133,15 @@ class DatapointsStore extends BaseDatapointsStore { | |||
runInAction(() => { | |||
this.loading = false; | |||
}); | |||
|
|||
const response2 = (await request({ |
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.
We actually don't need the published_datapoints endpoint anymore, do we? Want to remove the code here and also from the backend?
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 sorry, the PR isn't ready for review yet so hadn't removed this yet!
`; | ||
|
||
export const AllMetricsButton = ({ |
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, does this belong in a styles file?
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.
Good question, I think for now I'll leave these out of the styles file!
margin-left: 8px; | ||
`; | ||
|
||
export const MetricOverviewActionShareButton = () => ( |
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.
similar, do these belong in a styles file? not sure what best practice is
Sorry, I meant to open this as a draft PR! Videos will be coming soon. Just opening the PR for transparency, though I was planning on adding the modals before removing the WIP and asking for review |
Though I was also thinking of keeping this as a small PR and adding the modals in a follow-up |
Got it! No worries, sorry for jumping the gun on review :)
That actually makes sense to me! You know how I love small / incremental PRs :) |
In that spirit, I'm keeping this PR small and as-is, will be putting new work following up this PR! |
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.
Love it! +1 for small/incremental PRs!
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.
Code LGTM, Terry! I wasn't able to run it for some reason - I ran yarn install
and yarn run dev
but the site doesn't load on localhost:3000 nor localhost:3001. Wondering if it's just me/something in my local env.
What's the error you're getting? does yarn dev compile successfully? |
No errors at all actually - and everything compiles successfully - just the "This site can’t be reached. localhost refused to connect." screen. |
Even across different browsers? I was having that issue on chrome until I realized I had network disabled in the chrome debugging tools... |
Description of the change
Implemented the new left side panel and all metrics button + header bar. Gave the buttons a hover state.
There's a tiny unfinished modal for the about page, that will be fleshed out in a later PR!
To follow up in later PRs:
Type of change
Related issues
Closes #XXXX
Checklists
Development
This box MUST be checked by the submitter prior to merging:
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: