Skip to content

Conversation

@jepser
Copy link
Contributor

@jepser jepser commented Sep 21, 2018

This PR fixes:

  • Filter parameters for forms.list, responses.list are fixed
  • pageSize is now camelcase, deprecated page_size

Copy link

@epallerols epallerols left a comment

Choose a reason for hiding this comment

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

only doubt is that if buildUrlWithParams('http://mandarina.com', { a: 0, b: 3 }) should return "http://mandarina.com?b=3" or "http://mandarina.com?a=0&b=3"

Whit the current implementation returns just b. I am not sure if it is desired

}))
}

export const buildUrlWithParams = (url, params = {}) => {

Choose a reason for hiding this comment

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

Another approach could be to filter out the falsy values first:

export const buildUrlWithParams = (url, params = {}) => {
  const queryParams = Object.keys(params)
    .filter((k) => params[k])
    .map(k => `${encodeURIComponent(k)}=${encodeURIComponent(params[k])}`)
    .join('&')
   return queryParams ? `${url}?${queryParams}` : url
}

Copy link
Contributor Author

@jepser jepser Sep 22, 2018

Choose a reason for hiding this comment

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

This is because filter and map will iterate over the values twice, and reduce will do it only once, said that your solution looks more readable and since the parameters won't be more than 10 elements, I will implement your suggestion, thanks :)

@jepser jepser merged commit 5c5844f into master Sep 22, 2018
@jepser jepser deleted the fix-responses-after branch September 22, 2018 03:35
@jepser
Copy link
Contributor Author

jepser commented Sep 22, 2018

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants