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.

Sign up for GitHub

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

Jump to bottom

simplify doc explorer components #2477

Merged
merged 6 commits into from
Jun 14, 2022
Merged

simplify doc explorer components #2477

merged 6 commits into from
Jun 14, 2022

Conversation

thomasheyenbrock
Copy link
Collaborator

@thomasheyenbrock thomasheyenbrock commented Jun 4, 2022 •
edited
Loading

Instead of passing down the schema and callbacks for clicking types and fields via props, we can

  • consume the schema context directly in the component where we need the schema
  • push to the nav stack directly in the TypeLink and FieldLink components

ℹ️ much more easy to review with hidden whitespace ℹ️

No changeset needed as non of the components where the props changed are public.

@changeset-bot
Copy link

changeset-bot bot commented Jun 4, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: 19a271a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@codecov
Copy link

codecov bot commented Jun 4, 2022 •
edited
Loading

Codecov Report

Merging #2477 (19a271a) into main (2d91916) will increase coverage by 2.66%.
The diff coverage is 24.53%.

@@            Coverage Diff             @@
##             main    #2477      +/-   ##
==========================================
+ Coverage   65.70%   68.37%   +2.66%     
==========================================
  Files          85       71      -14     
  Lines        5106     4167     -939     
  Branches     1631     1384     -247     
==========================================
- Hits         3355     2849     -506     
+ Misses       1747     1313     -434     
- Partials        4        5       +1
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% <ø> (ø)
...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% <ø> (ø)
packages/graphiql-react/src/editor/whitespace.ts 100.00% <ø> (ø)
packages/graphiql-react/src/utility/debounce.ts 0.00% <0.00%> (ø)
... and 95 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 94f4876...19a271a. Read the comment docs.

@thomasheyenbrock
Copy link
Collaborator Author

Very nice comments @timsuchanek! 👏 Didn't look too closely at the implementation of the component yet. But while we're at it we can as well do this 👍

@thomasheyenbrock
Copy link
Collaborator Author

After looking into it I could only do the last one for typescript reasons

timsuchanek
timsuchanek approved these changes Jun 5, 2022
View reviewed changes
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.

Nice, LGTM now!

@acao
Copy link
Member

acao commented Jun 5, 2022 •
edited
Loading

@thomasheyenbrock since there are so many projects who use only the DocExplorer export directly (which is documented usage), can we add something to the graphiql/react readme to show how to use the schema context and the DocExplorer component standalone? Or make the schema prop optional?

CC @benjie

@benjie
Copy link
Member

benjie commented Jun 9, 2022

Slightly off topic for this PR, but on the topic of the doc explorer; in the new design will there be a prop we can add where it turns off the checkboxes/etc so that we can still use the component as a doc explorer without it building queries/etc?

@thomasheyenbrock
Copy link
Collaborator Author

@acao good shout! I added back the schema prop to the DocExplorer for now and marked it as deprecated to be removed in v2.

@thomasheyenbrock
Copy link
Collaborator Author

@benjie I can imagine that we would offer multiple versions of the doc explorer as plugins where one will be built-in (either the current one or the more advanced new one, tbd 😅 )

@thomasheyenbrock thomasheyenbrock merged commit 5268c9a into graphql:main Jun 14, 2022
@thomasheyenbrock thomasheyenbrock deleted the refactor/simplify-doc-explorer branch June 14, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimaMachina dimaMachina dimaMachina left review comments

@timsuchanek timsuchanek timsuchanek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasheyenbrock @acao @benjie @timsuchanek @dimaMachina