-
Notifications
You must be signed in to change notification settings - Fork 13
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 skeleton pages for user dashboard routes #156
Add skeleton pages for user dashboard routes #156
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.
This is great. One question.
@@ -0,0 +1,21 @@ | |||
import { Heading, Text, Link } from '@chakra-ui/react'; |
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.
What are the rules about using Chakra UI Link vs. Remix Link? I wonder if we should prefer the router's link component?
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.
Oh, I simply used a Link
here to render a <a>
element which links to #114 . This is so that anyone working on the page can see where it came from. I felt the on other pages it was more obvious what they were about
Should we have a separate heading page so we don't have to recycle it for each route? |
You mean a separate component for the header that appear on the figma designs? |
yeah... I was wondering how you'll be handling displaying the header across all the routes? |
Hmm, yea I hadn't thought of that. I will need to take a look to understand the best way to do this |
I have added a common header component for both |
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
app/routes/certificates.tsx
Outdated
export default function CertificatesRoute() { | ||
return ( | ||
<div> | ||
<Header /> |
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.
Probably this can be put higher in the router order, since every page will have this header, no? I'd do what you're doing here at the very top of the hierarchy, and render all the other routes into the outlet.
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.
would rendering it at the top affect the login page?
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.
True, need to put it in the right spot. We could also decide to always show the header, and remove the user info from the right. Remix has useMatches()
that lets you get data loaded in any route layer, which you can use to get a user object or something without passing it around via props. We could use this to know if the user is logged in or not, and change how we show things.
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.
There is also a way to do this purely based on the routes folder structure. I found this useful blog about it.
I have changed it so the header will be display on all the routes except login
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.
Neat, I didn't know about 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.
Looks good to me
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.
LGTM
Resolves #138
Added basic pages for the following routes (as mentioned in the issue):
/domains
/domains/new
/domains/:id
/certificate
/certificate/instructions
Also added a component for the header that will display on top of these pages. See image here
These are basic pages which can be styled later based on the figma designs
Steps to test