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

Allow passing qs parameter to cy.visit to set querystring #5040

Merged
merged 8 commits into from
Sep 17, 2019
Merged

Allow passing qs parameter to cy.visit to set querystring #5040

merged 8 commits into from
Sep 17, 2019

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Aug 26, 2019 •
edited
Loading

Pre-merge Tasks

@flotwig flotwig mentioned this pull request Aug 26, 2019
Merged
@flotwig flotwig requested a review from a team August 26, 2019 16:49
@flotwig flotwig mentioned this pull request Aug 26, 2019
Closed
@cypress
Copy link

cypress bot commented Aug 26, 2019 •
edited
Loading


Test summary

3344 • 0 • 47 • 0

Run details
Project cypress
Status Passed
Commit 98fc2cf
Started Sep 17, 2019 7:57 PM
Ended Sep 17, 2019 8:01 PM
Duration 04:11 💡
OS Linux Debian - 8.10
Browser Multiple

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

jennifer-shehane
jennifer-shehane requested changes Aug 26, 2019
View reviewed changes
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

I don't personally like the name of this argument being qs. It doesn't seem immediately obvious what qs stands for.

I liked the original issues suggestion of passing in the arg as params.

BUT, I'm also perhaps not as familiar with other existing APIs in this space - so if this is a really common convention then 🤷‍♀

packages/driver/test/cypress/integration/commands/navigation_spec.coffee Show resolved Hide resolved
@flotwig
Copy link
Contributor Author

flotwig commented Aug 26, 2019

I don't personally like the name of this argument being qs. It doesn't seem immediately obvious what qs stands for.

I liked the original issues suggestion of passing in the arg as params.

BUT, I'm also perhaps not as familiar with other existing APIs in this space - so if this is a really common convention then woman_shrugging

I'm fine with params, I've seen it used before too. Just because request uses qs doesn't mean we have to too especially since it's a little obscure.

@flotwig flotwig changed the title Allow passing qs parameter to cy.visit to set querystring Allow passing params parameter to cy.visit to set querystring Aug 26, 2019
@flotwig
Copy link
Contributor Author

flotwig commented Aug 26, 2019

Actually, cy.request() uses qs, so we should probably use that here too.

@flotwig flotwig changed the title Allow passing params parameter to cy.visit to set querystring Allow passing qs parameter to cy.visit to set querystring Aug 26, 2019
@jennifer-shehane
Copy link
Member

@flotwig Oh 😞 I didn't realize this was supported in cy.request, qs it is then.

@flotwig flotwig merged commit 9f717fe into develop Sep 17, 2019
@flotwig flotwig deleted the issue-5034 branch September 17, 2019 20:06
@flotwig flotwig restored the issue-5034 branch September 17, 2019 20:07
@kuceb kuceb mentioned this pull request Sep 17, 2019
Closed
grabartley pushed a commit to grabartley/cypress that referenced this pull request Oct 6, 2019
@flotwig @grabartley
Allow passing qs parameter to cy.visit to set querystring (cypress-io…
edbdf48 
…#5040)

* allow passing qs param to cy.visit

* add types

* qs -> params, merge with url, throw errors on invalid type, add tests

* params -> qs

* cleanup

* fix test
@semantic-commit semantic-commit bot mentioned this pull request Nov 21, 2019
Closed
@jennifer-shehane jennifer-shehane mentioned this pull request May 7, 2020
Closed
@flotwig flotwig deleted the issue-5034 branch January 24, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kuceb kuceb kuceb approved these changes

@jennifer-shehane jennifer-shehane jennifer-shehane approved these changes

@brian-mann brian-mann Awaiting requested review from brian-mann

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

Successfully merging this pull request may close these issues.

cy.visit() and URL encoding
3 participants
@flotwig @jennifer-shehane @kuceb