fix(external-sources): done button loading state behaviour #770
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant ExternalSource
participant Spinner
User->>ExternalSource: Select files
ExternalSource->>ExternalSource: Validate file selection
alt Files Selected
ExternalSource->>Spinner: Show loading indicator
ExternalSource->>ExternalSource: Enable "Done" button
else No Files
ExternalSource->>Spinner: Hide loading indicator
ExternalSource->>ExternalSource: Disable "Done" button
end
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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: 1
🧹 Nitpick comments (3)
blocks/Spinner/Spinner.js (1)
3-3: Class definition looks good
DefiningSpinneras a subclass ofBaseComponentappears appropriate. Consider adding documentation if you foresee more functionality in the future.blocks/ExternalSource/ExternalSource.js (1)
36-36:doneBtnTextClassnaming
Using a dedicated property for toggling text visibility is fine. Consider naming it to reflect its purpose (e.g.,doneBtnTextVisibility) to improve clarity.blocks/ExternalSource/external-source.css (1)
82-85: LGTM! Consider using CSS custom properties for visibility states.The implementation correctly uses
visibility: hiddento preserve layout spacing while hiding content. The addition ofpointer-events: noneis a good practice for hidden elements.Consider defining these visibility states using CSS custom properties for better maintainability:
uc-external-source .uc-done-btn > span.uc-hidden { - visibility: hidden; + visibility: var(--uc-visibility-hidden, hidden); pointer-events: none; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
blocks/ExternalSource/ExternalSource.js(4 hunks)blocks/ExternalSource/external-source.css(1 hunks)blocks/Spinner/Spinner.js(1 hunks)blocks/Spinner/spinner.css(1 hunks)blocks/themes/uc-basic/index.css(1 hunks)index.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- blocks/Spinner/spinner.css
🔇 Additional comments (8)
blocks/Spinner/Spinner.js (2)
1-2: Import statement and file structure look solid
No issues found; the import from @symbiotejs/symbiote is correct, and the file's initial structure is clean.
5-5: Template definition is concise
The inline HTML for the spinner is straightforward and reusable. Ensure that any additional spinner markup variants are documented if you plan to extend functionality.
index.js (1)
35-35: Export statement is properly added
Exporting Spinner from this file correctly centralizes component exports. This increases discoverability and consistency in your component library.
blocks/ExternalSource/ExternalSource.js (4)
31-31: New property for done button state is well introduced
isDoneBtnEnabled clarifies the condition for enabling the "Done" button. Keep it up-to-date with the selection logic to avoid confusion.
151-153: Conditional logic for the selection
These lines ensure the spinner and text states are synchronized with isSelectionReady. The approach is correct, but remember to thoroughly test edge cases (e.g., single-file selection, empty selection).
247-247: Reset logic
Clearing isDoneBtnEnabled to false on reset is consistent with overall state management. This helps ensure no stale UI states remain.
275-278: Button layout and spinner usage
The button uses the spinner element alongside toggling text visibility. This provides clear user feedback while loading. The implementation is cohesive and follows good UI/UX practices.
blocks/themes/uc-basic/index.css (1)
33-33: CSS import for spinner
Importing spinner.css here is consistent with the theme structure. Ensure the file path remains valid if the folder layout changes in the future.
| uc-external-source .uc-done-btn > uc-spinner { | ||
| position: absolute; | ||
| width: 100%; | ||
| height: 100%; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
ARIA attributes are needed for loading state
The code review was correct. The ExternalSource component uses a spinner but doesn't implement the recommended ARIA attributes for loading state. Specific findings:
- The button with class
uc-done-btncontains a<uc-spinner>component - The spinner visibility is controlled by
isSelectionReadybinding - No ARIA attributes are set when the loading state changes
To improve accessibility, add these attributes when the spinner is shown:
- In the button element, add
aria-busy="true"when!isSelectionReady - Add an appropriate
aria-labelto indicate the loading state
Example implementation:
<button type="button" class="uc-done-btn uc-primary-btn"
set="onclick: onDone;
@disabled: !isDoneBtnEnabled;
@aria-busy: !isSelectionReady;
@aria-label: !isSelectionReady ? 'Loading, please wait...' : null">🔗 Analysis chain
Consider adding ARIA attributes for loading state.
The spinner positioning and centering implementation is solid. However, to improve accessibility, ensure the button has appropriate ARIA attributes when in loading state.
The CSS looks good, but verify that the JavaScript implementation includes these ARIA attributes on the button when loading:
button.setAttribute('aria-busy', 'true');
button.setAttribute('aria-label', 'Loading, please wait...');Let's verify if these ARIA attributes are already implemented:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for aria-busy attribute usage in button elements
ast-grep --pattern 'setAttribute($attr, $val)' | grep -i 'aria-busy'
# Search for loading-related ARIA labels
rg -i "aria-label.*load"
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for ARIA attributes and loading states
# Search for any aria-busy usage
rg -i "aria-busy"
# Search for aria attribute setting in JavaScript
rg "setAttribute.*aria"
# Look for the button component implementation
fd -e js -e ts "button" --exec grep -l -i "loading\|spinner"
# Search specifically in the ExternalSource component
fd -e js -e ts "ExternalSource" --exec cat {}
Length of output: 8669
Description
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Style
Documentation