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

fix(external-sources): done button loading state behaviour #770

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(external-sources): done button loading state behaviour #770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions blocks/ExternalSource/ExternalSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@
total: 0,

isSelectionReady: false,
isDoneBtnEnabled: false,
couldSelectAll: false,
couldDeselectAll: false,
showSelectionStatus: false,
counterText: '',
doneBtnTextClass: 'uc-hidden',

onDone: () => {
for (const message of this.$.selectedList) {
Expand Down Expand Up @@ -87,7 +89,7 @@
this.mountIframe();
},
});
this.sub('*currentActivityParams', (val) => {

Check warning on line 92 in blocks/ExternalSource/ExternalSource.js

View workflow job for this annotation

GitHub Actions / build 

'val' is defined but never used
if (!this.isActivityActive) {
return;
}
Expand All @@ -103,7 +105,7 @@
this.$.showSelectionStatus = multiple;
});

this.subConfigValue('localeName', (val) => {

Check warning on line 108 in blocks/ExternalSource/ExternalSource.js

View workflow job for this annotation

GitHub Actions / build 

'val' is defined but never used
this.setupL10n();
});
}
Expand Down Expand Up @@ -146,7 +148,9 @@
);

this.set$({
doneBtnTextClass: message.isReady ? '' : 'uc-hidden',
isSelectionReady: message.isReady,
isDoneBtnEnabled: message.isReady && message.selectedFiles.length > 0,
showSelectionStatus: message.isMultipleMode && message.total > 0,
couldSelectAll: message.selectedCount < message.total,
couldDeselectAll: message.selectedCount === message.total,
Expand Down Expand Up @@ -240,7 +244,7 @@
this.set$({
selectedList: [],
total: 0,
isSelectionReady: false,
isDoneBtnEnabled: false,
couldSelectAll: false,
couldDeselectAll: false,
showSelectionStatus: false,
Expand Down Expand Up @@ -268,12 +272,10 @@
<button type="button" set="onclick: onSelectAll; @hidden: !couldSelectAll" l10n="select-all"></button>
<button type="button" set="onclick: onDeselectAll; @hidden: !couldDeselectAll" l10n="deselect-all"></button>
</div>
<button
type="button"
class="uc-done-btn uc-primary-btn"
set="onclick: onDone; @disabled: !isSelectionReady"
l10n="done"
></button>
<button type="button" class="uc-done-btn uc-primary-btn" set="onclick: onDone; @disabled: !isDoneBtnEnabled;">
<uc-spinner set="@hidden: isSelectionReady"></uc-spinner>
<span l10n="done" set="@class: doneBtnTextClass"></span>
</button>
</div>
</div>
`;
14 changes: 14 additions & 0 deletions blocks/ExternalSource/external-source.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,17 @@ uc-external-source uc-activity-header {
uc-external-source uc-activity-header .uc-close-btn {
pointer-events: auto;
}

uc-external-source .uc-done-btn > span.uc-hidden {
visibility: hidden;
pointer-events: none;
}

uc-external-source .uc-done-btn > uc-spinner {
position: absolute;
width: 100%;
height: 100%;
display: flex;
align-items: center;
justify-content: center;
}
Comment on lines +87 to +94
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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-btn contains a <uc-spinner> component
  • The spinner visibility is controlled by isSelectionReady binding
  • 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-label to 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

5 changes: 5 additions & 0 deletions blocks/Spinner/Spinner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { BaseComponent } from '@symbiotejs/symbiote';

export class Spinner extends BaseComponent {}

Spinner.template = /* HTML */ ` <div class="uc-spinner"></div> `;
18 changes: 18 additions & 0 deletions blocks/Spinner/spinner.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
@keyframes uc-spinner-keyframes {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}

.uc-spinner {
width: 1em;
height: 1em;
border: solid 2px transparent;
border-top-color: currentColor;
border-left-color: currentColor;
border-radius: 50%;
animation: uc-spinner-keyframes 400ms linear infinite;
}
1 change: 1 addition & 0 deletions blocks/themes/uc-basic/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
@import url('../../CloudImageEditor/index.css');
@import url('../../CloudImageEditorActivity/index.css');
@import url('../../Select/select.css');
@import url('../../Spinner/spinner.css');

/* POST RESET */
@import url('post-reset.css');
1 change: 1 addition & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export { FormInput } from './blocks/FormInput/FormInput.js';
export { ActivityHeader } from './blocks/ActivityHeader/ActivityHeader.js';
export { Select } from './blocks/Select/Select.js';
export { Copyright } from './blocks/Copyright/Copyright.js';
export { Spinner } from './blocks/Spinner/Spinner.js';

// Solutions:
export { FileUploaderRegular } from './solutions/file-uploader/regular/FileUploaderRegular.js';
Expand Down
Loading