initial code #1
initial code #1
Conversation
|
Approved the PR by mistake. |
src/lib/HasuraGraphiQL.tsx
Outdated
| </table> | ||
| <div | ||
| className="graphiql-container" | ||
| style={{ |
There was a problem hiding this comment.
Why isn't this a class? I don't see anything variable here.
There was a problem hiding this comment.
For some reason, this didn't work inside the 'graphiql-container` class even after adding !important. Probably some CSS specificity thing I don't fully understand. So, left this as such.
There was a problem hiding this comment.
That's probably because graphiql-container is a graphiql specific class and it might not not be trivial to override it.
Would work if we write a separate class and pass the className of this div as graphiql-container custom-class. This doesn't work either?
|
@beaussan do you mind signing off on this too? |
|
Have completely rewritten the code. Sorry about the re-review effort but the new code is shorter and simpler than the original and hopefully, easier to review. Design ChoicesData Flow
Batched State UpdatesI have used composite objects along with useState to store related data. Felt this was easier to understand than using useReducer. |
| import * as React from "react"; | ||
| import { IconChevronRight, IconChevronDown } from "./Icons"; | ||
|
|
||
| export default function Collapsible({ |
src/lib/HasuraGraphiQL.tsx
Outdated
| </table> | ||
| <div | ||
| className="graphiql-container" | ||
| style={{ |
There was a problem hiding this comment.
That's probably because graphiql-container is a graphiql specific class and it might not not be trivial to override it.
Would work if we write a separate class and pass the className of this div as graphiql-container custom-class. This doesn't work either?
| }) { | ||
| const [collapsed, setCollapsed] = React.useState(false); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Nitpicking here, but did you consider https://www.radix-ui.com/docs/primitives/components/collapsible ? It would cover all of the acceptability concern here
There was a problem hiding this comment.
Didn't really look into the application from an accessibility perspective. Found some more a11y problems. Will add a new GitHub issue to address this. Will update this comment with issue# in a while.
There was a problem hiding this comment.
Captured this concern in #2
Intro
This PR implements part of the RFC https://github.com/hasura/lux/blob/main/docs/rfcs/20210920_public_graphiql.md
(the second part is implemented by - https://github.com/hasura/lux/pull/3302 )
Design Notes
The UI/UX mimics the console API Explorer page.
Screenshot of API explorer -
Screenshot of this component -
The 'Relay' control is hidden for now as it's still unclear when we should show it. ( discussed with @wawhal )
The notification styles are also copied from the console UI/UX. (discussed with @martin-hasura )
We have used cypress component testing for the unit tests. RTL was considered and later discarded as it required mocking too many browser libs in this case.
Setup Instructions
See README.md