Fix/spark 564418 fixing download button for shared files and screenshot #853
Conversation
WalkthroughThis update introduces file sharing capabilities within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebexActivity
participant Browser
User->>WebexActivity: View activity with attached files
WebexActivity->>User: Render file previews and download buttons
User->>WebexActivity: Click download button for a file
WebexActivity->>Browser: Trigger file download via anchor element
Browser-->>User: File is downloaded
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (3)
src/components/WebexActivity/WebexActivity.scss (1)
28-58: Good accessibility implementation, but inconsistent naming conventionThe CSS implementation correctly handles both hover and keyboard focus states for the download button, ensuring accessibility. The transitions and positioning create a clean user experience.
However, the class naming doesn't follow the BEM pattern used elsewhere in the codebase (e.g.,
.#{$C}__content).Consider refactoring the class names to match the existing BEM convention:
-.share-file-wrapper { +.#{$C}__share-file-wrapper { position: relative; display: inline-block; margin-top: 8px; outline: none; } -.shared-screenshot { +.#{$C}__shared-screenshot { max-width: 100%; border-radius: 4px; display: block; } // Download button initially hidden, appears on hover or keyboard focus -.webex-share-item-actions { +.#{$C}__share-item-actions { position: absolute; bottom: 8px; right: 8px; opacity: 0; visibility: hidden; transition: opacity 0.3s ease; background: rgba(0, 0, 0, 0.5); border-radius: 4px; padding: 4px; } -.share-file-wrapper:hover .webex-share-item-actions, -.share-file-wrapper:focus-within .webex-share-item-actions { +.#{$C}__share-file-wrapper:hover .#{$C}__share-item-actions, +.#{$C}__share-file-wrapper:focus-within .#{$C}__share-item-actions { opacity: 1; visibility: visible; }src/components/WebexActivity/WebexActivity.jsx (2)
50-78: Replace emoji with proper icon from design systemThe implementation provides good accessibility with proper ARIA attributes and keyboard navigation, but uses an emoji instead of a proper icon.
<button type="button" className="md-button md-button--32" aria-label={`Download ${file.displayName}`} onClick={() => downloadFile(file.url, file.displayName)} > - ⬇️ + <Icon name="download" /> </button>Also, ensure the button class names match the project's design system - "md-button" suggests Material Design, but verify if this is the correct convention for this project.
54-58: Ensure consistent class naming with component CSSTo match the class naming used in the SCSS file with the JSX implementation:
<div - className="share-file-wrapper" + className={sc("share-file-wrapper")} tabIndex="0" role="group" key={file.url} > <img src={file.url} alt={file.displayName || "shared file"} - className="shared-screenshot" + className={sc("shared-screenshot")} /> - <div className="webex-share-item-actions"> + <div className={sc("share-item-actions")}>This would make the styling consistent with your component helper function and make refactoring easier in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/adapters/MeetingsJSONAdapter/controls/SwitchMicrophoneControl.js(0 hunks)src/adapters/MeetingsJSONAdapter/controls/SwitchSpeakerControl.js(0 hunks)src/components/WebexActivity/WebexActivity.jsx(2 hunks)src/components/WebexActivity/WebexActivity.scss(1 hunks)src/components/inputs/Dropdown/Dropdown.jsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/adapters/MeetingsJSONAdapter/controls/SwitchSpeakerControl.js
- src/adapters/MeetingsJSONAdapter/controls/SwitchMicrophoneControl.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test_and_storybook
🔇 Additional comments (1)
src/components/inputs/Dropdown/Dropdown.jsx (1)
57-64: Improved code clarity through explicit conditional handlingThe refactoring from a ternary expression to explicit if/else conditions with dedicated function calls makes the toggle behavior more readable and maintainable.
| function downloadFile(url, name) { | ||
| const link = document.createElement("a"); | ||
| link.href = url; | ||
| link.download = name; | ||
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error handling for the download functionality
The download function correctly creates and triggers a temporary link, but lacks error handling for potential failures.
function downloadFile(url, name) {
+ try {
const link = document.createElement("a");
link.href = url;
link.download = name;
document.body.appendChild(link);
link.click();
document.body.removeChild(link);
+ } catch (error) {
+ console.error('Failed to download file:', error);
+ // Consider showing user-friendly error notification
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function downloadFile(url, name) { | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.download = name; | |
| document.body.appendChild(link); | |
| link.click(); | |
| document.body.removeChild(link); | |
| } | |
| function downloadFile(url, name) { | |
| try { | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.download = name; | |
| document.body.appendChild(link); | |
| link.click(); | |
| document.body.removeChild(link); | |
| } catch (error) { | |
| console.error('Failed to download file:', error); | |
| // Consider showing user-friendly error notification | |
| } | |
| } |
…r_Shared_Files_And_Screenshot
Shreyas281299
left a comment
Shreyas281299
left a comment
There was a problem hiding this comment.
- Please add some testing recording showcasing the changes
- The JIRA doesnt mention adding a new download functionality it is just asking us to add the accessibility on focus. Why is the method
downloadFilerequired? - Please follow the semantic way for the PR title -
https://github.com/semantic-release/commit-analyzer?tab=readme-ov-file#rules-matching
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); |
There was a problem hiding this comment.
We should avoid using document.body, you can directly add this to component being returned
3 participants
What's Changed
Issue Addressed
Previously, the shared screenshots or file thumbnails were not accompanied by any visible or accessible download option.