-
Notifications
You must be signed in to change notification settings - Fork 111
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
Developer Tools page #2046
Developer Tools page #2046
Conversation
Hey @MaxGhenis , |
@bharateshwq This is looking great! Thanks for your work on it. Could you just do the following on the design:
I might also ask for a couple changes to the text of the header afterward |
Hey @anth-volk, |
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.
Thanks for your work on this @bharateshwq, this is looking good. There are a few changes I was hoping to request:
- At the moment, this code places a wrapper around the API status and simulations pages. Could you remove that, including the "back" arrow? The pages themselves should actually be unchanged.
- Could you link this page into the footer with the tag "Developer tools"?
- Minor, but could you add padding to the right of the text within the link boxes at the mobile viewpoint? In the image below, you can see how the word "Simulations" runs into the button.
I might also request some textual changes, but am hoping to do that at the end.
.eslintrc.json
Outdated
"allow": ["error", "warn", "info", "table"] | ||
} | ||
] | ||
"jsx-a11y/no-static-element-interactions": "off" |
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.
Could you remove this before your final commit?
Hey @anth-volk, I'll definitely fix that. Do you prefer having the |
@bharateshwq I think the way you layered the URLs makes sense, but I also think their browser back button should be able to handle that navigation action, as I believe |
Yes @anth-volk, React Router does write to the history, so using the browser's back button would work for navigation. However, I'll be removing the back button from the UI. Additionally, the current(master) API status and simulation headers have different designs, which I feel may not appear consistent to the user. |
@bharateshwq That is a good point. Considering these are internal developer tools, I don't think it's too big of an issue, but certainly warrants opening a separate issue. I can't remember their designs well enough, but I'm guessing the API status page is more out-of-line with our current design. If you'd like to take it on after this one, we'd greatly appreciate it. |
Hey @anth-volk, I've made the updates as requested and am awaiting your feedback. Thanks! |
@bharateshwq Thanks, I'm hoping to get to this either tonight or tomorrow |
30fb351
to
f5b5129
Compare
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.
Hey @bharateshwq. Thanks again for your work on this, this will be a major benefit for internal development. I've left a couple of minor change requests, and also took the liberty of cleaning up a merge conflict/linting. After this, we should be good to merge!
src/layout/Footer.jsx
Outdated
@@ -61,6 +61,11 @@ function LinkSection() { | |||
label: "Terms and Conditions", | |||
isInternal: true, | |||
}, | |||
{ | |||
link: `/${countryId}/developer-tools`, | |||
label: "Developer tools", |
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.
label: "Developer tools", | |
label: "Developer Tools", |
src/pages/DeveloperHome.jsx
Outdated
const { pathname } = useLocation(); | ||
|
||
useEffect(() => { | ||
console.log("Scrolling to top on pathname change:", pathname); |
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.
console.log("Scrolling to top on pathname change:", pathname); |
src/pages/DeveloperHome.jsx
Outdated
); | ||
} | ||
|
||
function ToolsStruct({ |
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.
Just personal preference: structs do exist in JS (albeit more as a concept) could you change this name?
Hey @anth-volk , |
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.
Thanks for your work on this @bharateshwq, it's looking great.
Earlier, when I had asked that you remove the enclosing features, it was because it created a wrapper that added a "back" button and created two footers. We actually still want the "header" (the navbar) and the footer, just not the back button. Could you add the navbar back into these two pages?
After that, this will be good to go.
Hey, |
Co-authored-by: Anthony V. <[email protected]>
Co-authored-by: Anthony V. <[email protected]>
Co-authored-by: Anthony V. <[email protected]>
ed70bca
to
f95945e
Compare
@bharateshwq I've taken the liberty of addressing some merge conflicts in the PR. I'll approve and merge once tests pass. |
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.
Thank you very much @bharateshwq! Merging now.
Description
Fixes #2035
Created basic wire frame design for how the developer tools should look like while keeping in consideration the design language of policy Engine
Let me know if there are changes required for this
This in no way is the final commit that i expect to be merged there are a lot of optimization, responsiveness tasks and restructuring to be
Screenshots