fix ics download button #1381
fix ics download button #1381
Conversation
|
@a-mnich is attempting to deploy a commit to the rallly Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/web/src/components/add-to-calendar-button.tsx (1)
120-137: LGTM: ICS download functionality correctly implementedThe ICS file download functionality has been correctly implemented in the new DropdownMenuItem. This change addresses the issue of the non-functional ICS download button (issue #1380) and aligns with the PR objectives.
The implementation is solid, creating a Blob from the ICS data, generating a download URL, and programmatically triggering the download. The file naming convention based on the event title is a good practice.
Consider adding basic error handling to improve robustness:
onClick={() => { const res = ics(calendarEvent); + try { // download the file const blob = new Blob([res], { type: "text/calendar" }); const url = URL.createObjectURL(blob); const link = document.createElement("a"); link.setAttribute("href", url); link.setAttribute( "download", `${title.toLocaleLowerCase().replace(/\s/g, "-")}.ics`, ); document.body.appendChild(link); link.click(); document.body.removeChild(link); + URL.revokeObjectURL(url); // Clean up the URL object + } catch (error) { + console.error("Error downloading ICS file:", error); + // Optionally, show a user-friendly error message + } }}This addition will handle potential errors gracefully and clean up the URL object after use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/web/src/components/add-to-calendar-button.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
apps/web/src/components/add-to-calendar-button.tsx (2)
Line range hint
60-65: LGTM: Button component correctly modifiedThe Button component has been correctly modified to serve solely as a trigger for the dropdown menu. This change aligns with the PR objective of fixing the ICS download button issue and improves the separation of concerns in the component.
Line range hint
1-146: Summary: PR objectives met, issue resolvedThis PR successfully addresses the issue of the non-functional ICS download button (issue #1380). The changes effectively relocate the
onClickevent handler for downloading the ICS file from theDropDownMenuTriggerto the correctDropdownMenuItem. This implementation ensures that the ICS file can be downloaded as expected when the user selects the "Download ICS File" option from the dropdown menu.The changes improve the component's structure and separation of concerns, aligning with good coding practices. The
AddToCalendarButtoncomponent now correctly serves as a trigger for the dropdown menu, while the actual download functionality is encapsulated within the appropriate menu item.These modifications should resolve the problem reported in issue #1380, where clicking the ICS Download button after finalizing an event resulted in no action. Users should now be able to successfully download calendar events in ICS format, which is essential for integrating with various calendar applications.
To ensure that the changes are consistent across the codebase, please run the following verification script:
This script will help identify any instances where ICS download logic might still be present in other components, ensuring that the fix is comprehensive.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hey @a-mnich, thanks for catching this and for submitting a fix. I'll work on adding some unit tests to make sure this doesn't break again. Thanks again for your help! |
2 participants
Description
The download button for the ICS file currently has no effect. The code for downloading the ICS file was attached to the
DropDownMenuTriggerinstead of the actualDropdownMenuItem. With this PR I am moving theonClickcode to the actualDropdownMenuItemthat should be responsible for the ICS download.resolves #1380
Checklist
Please check off all the following items with an "x" in the boxes before requesting a review.
calendar-linklibrary that is used. Unfortunately I wasn't able to actually test it locally as I wasn't able to find out how to login with the seeded users without setting up a SMTP catcher locally.Summary by CodeRabbit
New Features
Add to Calendarfunctionality with a dedicated dropdown menu for downloading ICS files.Bug Fixes