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

Change setSearch to handle SearchParams.t #176

Merged

Conversation

pedrobslisboa
Copy link
Collaborator

@pedrobslisboa pedrobslisboa commented Oct 29, 2024

Description

This PR changes the setSearch to handle SearchParams.t and add setSearchString to handle strings

New API

let setSearchAsString: (t, string) => t;
let setSearch: (t, SearchParams.t) => t;

Copy link
Member

@davesnx davesnx left a comment

Choose a reason for hiding this comment

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

Tiny nit, but overall looks perfect

@@ -214,8 +214,12 @@ let search = url => {
| _ => Some("?" ++ Uri.encoded_of_query(~scheme, query))
};
};
let setSearch = (t, string) => {
Uri.with_query(t, Uri.query_of_encoded(string));
let setSearchString = (t, searchString) => {
Copy link
Member

Choose a reason for hiding this comment

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

I would rename this to setSearchAsString

Comment on lines 221 to 222
let queryString = SearchParams.toString(searchParams);
setSearchString(t, queryString);
Copy link
Member

Choose a reason for hiding this comment

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

no need to transform toString here, Uri.with_query should do the job, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point

@davesnx davesnx merged commit ffa038c into ml-in-barcelona:main Oct 30, 2024
2 checks passed
davesnx added a commit that referenced this pull request Oct 30, 2024
… into rsc

* 'main' of github.com:ml-in-barcelona/server-reason-react:
  Handle DomProps with both jsx and html attribute name (#171)
  Change setSearch to handle SearchParams.t (#176)
  doc: add @@platform attribute documentation (#175)
  Support non-ascii on JS.String.toLowerCase and toUpperCase (#173)
  Fix snapshot test for external
  Upperbound reason to 3.10
  feat: add nested mel.obj support (#169)
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.

2 participants