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

Fix: URL params & error msg #119

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Fix: URL params & error msg #119

merged 3 commits into from
Oct 30, 2024
+30 −8

Conversation

wellyshen
Copy link
Contributor

@wellyshen wellyshen commented Oct 28, 2024 •
edited
Loading

Proposed Changes

  • Create an addQueryArg utility function to fix the issue
  • Add a 404 error msg

Testing Instructions

  • Check out this PR
  • Follow the development guidance to run the PR
  • Check the requested API URL, and the URL param still works the same:
截圖 2024-10-28 晚上8 27 45
  • Check the "View Profile" link, and the URL param still works the same:
截圖 2024-10-28 晚上8 28 05
  • For the error msg, code review should be enough

@wellyshen wellyshen self-assigned this Oct 28, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 28, 2024 •
edited
Loading

Size Change: +1.01 kB (+3.37%)

Total Size: 31 kB

Filename Size Change
dist/index.esm.js 6.67 kB +280 B (+4.38%)
dist/index.js 6.82 kB +278 B (+4.25%)
dist/index.react.js 8.74 kB +288 B (+3.41%)
dist/index.react.umd.js 4.87 kB +85 B (+1.78%)
dist/index.umd.js 3.94 kB +82 B (+2.13%)

compressed-size-action

@wellyshen wellyshen marked this pull request as ready for review October 28, 2024 12:40
@wellyshen wellyshen requested a review from a team October 28, 2024 12:40
This was linked to issues Oct 28, 2024
Hovercards: Add 404 error message #101
Closed
Hovercard assumes profile URL has no query parameters #93
Closed
wellyshen
wellyshen commented Oct 29, 2024
View reviewed changes
web/packages/hovercards/src/add-query-arg.ts
Comment on lines +9 to +16
export default function addQueryArg( url: string, key: string, value: string ): string {
const [ baseUrl, queryStr ] = url.split( '?' );
const queryParams = new URLSearchParams( queryStr || '' );

queryParams.set( key, value );

return `${ baseUrl }?${ queryParams.toString() }`;
}
Copy link
Contributor Author

@wellyshen wellyshen Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Although I'm not happy with the increased library size, I think it's an acceptable compromise between size (we've reduced the size already) and reliability.

@wellyshen wellyshen merged commit 6eb9ad4 into trunk Oct 30, 2024
2 checks passed
@wellyshen wellyshen deleted the fix/hovercards-issues branch October 30, 2024 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcheringer jcheringer jcheringer approved these changes

Assignees

@wellyshen wellyshen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hovercards: Add 404 error message Hovercard assumes profile URL has no query parameters
2 participants
@wellyshen @jcheringer