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

docs: update docs for issue 2208 switching to url-encoded style params #4620

Merged
merged 1 commit into from
Feb 13, 2020
Merged

docs: update docs for issue 2208 switching to url-encoded style params #4620

merged 1 commit into from
Feb 13, 2020

Conversation

deepakrkris
Copy link
Contributor

@deepakrkris deepakrkris commented Feb 11, 2020 •
edited
Loading

#2208

Opened to fix #4347 (comment)

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@deepakrkris deepakrkris requested a review from bajtos February 11, 2020 06:01
@deepakrkris deepakrkris requested a review from jannyHou February 11, 2020 06:44
dhmlau
dhmlau reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

Some grammar related comments.

docs/site/Parsing-requests.md Outdated
```

LoopBack has switched definition of json query params from the `exploded`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change:
LoopBack has switched the definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

docs/site/Parsing-requests.md Outdated
with `style: "deepObject"` only. Please note that this style does not preserve
encoding of primitive types, numbers and booleans are always parsed as strings.
At the moment, LoopBack supports both url-encoded and exploded values for json
query parameters. Please note that this style does not preserve encoding of
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change:
does not preserve the encoding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

docs/site/Parsing-requests.md Outdated

For example:
For instance, to filter api results from the GET '/todo-list' endpoint in the
Copy link
Member

Choose a reason for hiding this comment

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

maybe just to filter results (without api)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

@dhmlau dhmlau added the Docs label Feb 11, 2020
@dhmlau dhmlau mentioned this pull request Feb 11, 2020
Merged
7 tasks
jannyHou
jannyHou reviewed Feb 12, 2020
View reviewed changes
docs/site/Parsing-requests.md Outdated
```

LoopBack has switched the definition of json query params from the `exploded`,
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: how about move this sentence to the beginning of the section?

And I remember there is a breaking change of the syntax, can you explain more about how to upgrade after the release?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry never mind, found the change log in https://github.com/strongloop/loopback-next/blob/master/packages/rest/CHANGELOG.md#breaking-changes

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to moving this sentence up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

jannyHou
jannyHou reviewed Feb 12, 2020
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

You can run npm run lint:fix from the root folder to fix the lint error.
Otherwise LGTM 👍

nabdelgadir
nabdelgadir reviewed Feb 13, 2020
View reviewed changes
docs/site/Parsing-requests.md Outdated Show resolved Hide resolved
docs/site/Parsing-requests.md Outdated
```

LoopBack has switched the definition of json query params from the `exploded`,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to moving this sentence up

@deepakrkris deepakrkris merged commit 09c1b56 into master Feb 13, 2020
@deepakrkris deepakrkris deleted the docfix2208 branch February 13, 2020 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhmlau dhmlau dhmlau left review comments

@jannyHou jannyHou jannyHou left review comments

@nabdelgadir nabdelgadir nabdelgadir approved these changes

@emonddr emonddr emonddr approved these changes

@bajtos bajtos Awaiting requested review from bajtos

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

Successfully merging this pull request may close these issues.
5 participants
@deepakrkris @emonddr @jannyHou @dhmlau @nabdelgadir