Skip to content
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 editor context and useHeaderEditor hook to @graphiql/react #2404

Merged

Conversation

thomasheyenbrock
Copy link
Collaborator

Continuing moving over stuff to @graphiql/react, next up is a context that stores the editor instances. I also created a useHeaderEditor hook that contains all the logic for this editor and just returns the ref that needs to be passed to a div element.

Notes (also will add some inline comments):

  • I stopped with the header editor to keep the PR size under control, the other editors will follow soon!
  • Also the HeaderEditor component will be moved in a follow up, I'll try to do this together with design changes.
  • I copied over a bunch of utils around editors and completion. Once we moved all the editor logic over to @graphiql/react we can remove these utils from the graphiql package. (I'll mark these files with inline comments.)
  • I removed the headers state property from the GraphiQL component, we now always derive the current value by looking at the headers props and read the editor value if this prop value is undefined. The logic for keeping the editor value in sync is all part of the useHeaderEditor hook.

@changeset-bot
Copy link

changeset-bot bot commented May 13, 2022

🦋 Changeset detected

Latest commit: 63dd771

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
graphiql Minor
@graphiql/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #2404 (63dd771) into main (2d91916) will decrease coverage by 0.30%.
The diff coverage is 69.32%.

@@            Coverage Diff             @@
##             main    #2404      +/-   ##
==========================================
- Coverage   65.70%   65.40%   -0.31%     
==========================================
  Files          85       81       -4     
  Lines        5106     5185      +79     
  Branches     1631     1678      +47     
==========================================
+ Hits         3355     3391      +36     
- Misses       1747     1790      +43     
  Partials        4        4              
Impacted Files Coverage Δ
packages/codemirror-graphql/src/hint.ts 94.73% <ø> (ø)
packages/codemirror-graphql/src/lint.ts 100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts 47.05% <ø> (ø)
...kages/codemirror-graphql/src/utils/forEachState.ts 100.00% <ø> (ø)
packages/codemirror-graphql/src/utils/hintList.ts 95.65% <ø> (ø)
...ackages/codemirror-graphql/src/utils/info-addon.ts 1.02% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts 0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/hint.ts 89.70% <ø> (ø)
packages/codemirror-graphql/src/variables/mode.ts 79.48% <ø> (ø)
...aphiql/src/components/DocExplorer/DefaultValue.tsx 50.00% <ø> (ø)
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f695b1...63dd771. Read the comment docs.

@thomasheyenbrock thomasheyenbrock force-pushed the feat/header-editor-context branch from 7036641 to fd31afe Compare May 13, 2022 10:24
Copy link
Member

@timsuchanek timsuchanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@thomasheyenbrock thomasheyenbrock merged commit 029ddf8 into graphql:main May 17, 2022
@thomasheyenbrock thomasheyenbrock deleted the feat/header-editor-context branch May 17, 2022 14:27
@acao acao mentioned this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants