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

fix: use the default query on adding a new tab #3441

Merged
merged 9 commits into from
Aug 15, 2024
Merged

Conversation

cimdalli
Copy link
Contributor

When a new tab is added, it sets the query value as an empty string instead of defaultQuery value. Tab default query value should be defaultQuery variable if it's set on the component. Otherwise, it should be DEFAULT_QUERY constant.

@changeset-bot
Copy link

changeset-bot bot commented Oct 28, 2023

🦋 Changeset detected

Latest commit: a75583e

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

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

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

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 28, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@acao
Copy link
Member

acao commented Oct 28, 2023

makes sense to me. do any other @graphql/graphiql-maintainers have feelings about this?

@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.29%. Comparing base (40bbd61) to head (a75583e).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3441      +/-   ##
==========================================
- Coverage   67.30%   67.29%   -0.01%     
==========================================
  Files         121      121              
  Lines        7016     7017       +1     
  Branches     2263     2263              
==========================================
  Hits         4722     4722              
- Misses       2277     2278       +1     
  Partials       17       17              
Files Coverage Δ
packages/graphiql-react/src/editor/context.tsx 76.42% <100.00%> (+0.19%) ⬆️

... and 1 file with indirect coverage changes

@benjie
Copy link
Member

benjie commented Oct 28, 2023

I'm for this change 👍

@acao
Copy link
Member

acao commented Nov 15, 2023

two things:

  1. the cypress suite is failing on some tests you added
  2. the headers prop is a bit nuanced - however if it is set, then defaultHeaders should always be overridden. query, variables and headers prop, and soon extensions, should drive a controlled component state, at least I think? For 4.x I'm considering dropping all the controlled component props with a migration path to the @graphiql/react hooks instead, because it leads to a much more perfomant and easier to implement controlled component implementation

@acao
Copy link
Member

acao commented Jan 7, 2024

@cimdalli so it seems this clashes a bit with previously expected behavior:

image

when headers are supplied, they should only set the first tab, and not be used for new tabs apparently?

so we need to revert this for headers and do the same for query/defaultQuery. otherwise, it's a change in functionality and requires a new minor version

CC @dimaMachina who added tests for this expectation, what do you think?

@fieldsco
Copy link

Any update on this?

@dimaMachina dimaMachina changed the base branch from main to graphiql-v4 August 15, 2024 13:48
@dimaMachina dimaMachina changed the base branch from graphiql-v4 to main August 15, 2024 15:04
@dimaMachina dimaMachina changed the base branch from main to graphiql-v4 August 15, 2024 15:04
@dimaMachina dimaMachina changed the base branch from graphiql-v4 to main August 15, 2024 15:11
@dimaMachina dimaMachina merged commit 959ed21 into graphql:main Aug 15, 2024
2 checks passed
This was referenced Aug 15, 2024
dimaMachina added a commit that referenced this pull request Aug 15, 2024
* Use default query on adding new tab

* Fix eslint issue

* First try to use `query` and `headers` props

* add changeset

* upd

fix lint

this should fix cypress

fix

* Update .changeset/brown-tigers-nail.md

---------

Co-authored-by: Rikki Schulte <rikki.schulte@gmail.com>
Co-authored-by: Dimitri POSTOLOV <dmytropostolov@gmail.com>
@klippx
Copy link
Contributor

klippx commented Aug 19, 2024

Is everyone happy with this change? I find it quite a bit annoying to have the "welcome message" shown every time you open a new tab... 😞

Aside from a UX point of view, I also suddenly must find a way to run my playwright test to clear the entire editor before typing in a query after clicking "+" new tab button.

I know this is too much to ask, but I would almost like the old behaviour back (or a way to escape this new behaviour)

@dimaMachina
Copy link
Collaborator

@klippx you can set defaultQuery="" in <GraphiQL /> maybe?

@klippx
Copy link
Contributor

klippx commented Aug 20, 2024

I decided to keep the default query as we assert on text from it to ensure the user is logged in and page is fully loaded and that the test can be started. defaultQuery='' is an option, but makes it harder to know that the page is loaded. To get things working in playwright we simply clear the editor using a + directive before filling it with input: const editorTextbox = page.getByLabel('Query Editor').getByRole('textbox') await editorTextbox.press('ControlOrMeta+a') await editorTextbox.press('ControlOrMeta+x') It is more a matter of the UX: Each time a new tab is opened the defaultQuery is filled, which previously was only shown on the initial tab (something I have gotten used to). Imagine if your browser showed a tutorial screen every time you opened a new tab, or your favorite text editor showed a whole page of documentation every time you hit CMD+N. When creating a new tab - you want a blank screen, at least that's my opinion. However it is fine if it is shown on inital tab on initial load. Perhaps an option defaultQueryBehaviour = 'initial' | 'always' is what I would consider, and maybe this extra amount of complexity would be merited. [EDIT: this comment is not formatted well for some reason, I am not sure what is wrong with it]

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.

6 participants