-
Notifications
You must be signed in to change notification settings - Fork 12
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 boundary for connection #83
Conversation
<Container> | ||
<Logo alt={"Urql Eagle"} src={image} /> | ||
<Header>Waiting for exchange</Header> | ||
<Hint>Make sure {"you're"} using the Urql Devtools exchange!</Hint> |
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 be nice to add the link to the exchange here!
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 shout!
I'm going to leave it out for now just because I want to avoid giving the user the impression that interaction is required on this page (for most, it's only going to be present for a second or so).
Maybe this is something we can look into though!
<> | ||
<GlobalStyle /> | ||
<Container> | ||
<Logo alt={"Urql Eagle"} src={image} /> |
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 a tiny nit but this doesn't really need an alt
since it's not really giving any information/context to a screen reader user
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.
Gotta disagree with this one. Regardless of whether the image is important or not, it's valuable to the user to know what they're looking at.
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.
In general logos like these are considered decorative and are usually supplied an empty alt
text so the screen reader will know to skip it.
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 looks so nice, good work Andy. Left some nits but they don't really block the merge so will leave my approve already! Good work 💯
); | ||
}; | ||
|
||
export const AppRoutes: FC = () => { |
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.
I personally prefer having one component in one file, don't know how you perceive this. (more to spark a conversation 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.
I personally prefer having one component in one file
For the most part we've been taking this approach.
In this case I'm not sure there's much value in breaking it down into separate files - there's only two components and they would be (and were) one if it wasn't for the use of context/hooks.
Changes
Screenshots