va-button: add loading variation #1446

powellkerry · 2025-01-07T16:03:02Z

Chromatic

https://3460-button-loading--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

Add loading variation for button.
The button should:

act disabled (greyed out and disable all interactions)
display an animated loading icon, the icon should not be animated for reduced motion users
text should default to "Loading..." if the text prop is not set
Screen readers should announce "loading" when the loading prop is true and "Loading complete" when it is set to false

Closes 3460

QA Checklist

[ x] Component maintains 1:1 parity with design mocks
[ x] Text is consistent with what's been provided in the mocks
[ x] Component behaves as expected across breakpoints
Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
Designer has signed off on changes (if applicable. If not applicable provide reason why)
[ x] Tab order and focus state work as expected
[ x] Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
[ x] New components are covered by e2e tests; updates to existing components are covered by existing test suite
[ x] Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

Acceptance criteria

QA checklist has been completed
[ x] Screenshots have been attached that cover desktop and mobile screens

Definition of done

[ x] Documentation has been updated, if applicable
[ x] A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

powellkerry · 2025-01-07T16:06:47Z

packages/web-components/src/components/va-button/va-button.tsx

-          {buttonText}
-          {_continue && !back && <va-icon icon="navigate_far_next" />}
-        </button>
+          {/* This span must always be present for changes to be announced for the loading prop. It will not show visually or be read without content*/}


I don't love the idea of this span being rendered for all buttons. I have tested it and it does not announce anything when there is not content, but it would be nice to figure out a way to only render it if the button will be used for loading. Hoping someone has an idea for how to do that.

I don't think it's a big deal to have a span always in the markup but was there some limitation with just doing conditional JSX? Something like:

{this.loading && ( .... )}

danbrady

Design looks good. That animation is 🔥!

jamigibbs

Nothing blocking that I could see but I had just a few questions and comments. The animation sure is slick. 😎

jamigibbs · 2025-01-08T19:05:50Z

packages/web-components/src/components/va-button/va-button.tsx

@@ -50,6 +53,22 @@ export class VaButton {
   */
  @Prop({ reflect: true }) disabled?: boolean = false;

+  /**
+   * If `true`, the click event will not fire.


Is this description accurate for the loading prop?

Yes, this is handled the same way as the disabled prop by preventDefault() and preventPropogation() on line 168.

oh ok yes that makes sense. I was expecting it to mention that it will display the loading indicator too.

oh, I should add that! Thanks!

jamigibbs · 2025-01-08T19:08:15Z

packages/web-components/src/components/va-button/va-button.scss

+    animation: none;
+  }
+
+  @media (prefers-reduced-motion) {


This is a cool media feature. TIL!

jamigibbs · 2025-01-08T19:14:58Z

packages/web-components/src/components/va-button/va-button.tsx

-          {buttonText}
-          {_continue && !back && <va-icon icon="navigate_far_next" />}
-        </button>
+          {/* This span must always be present for changes to be announced for the loading prop. It will not show visually or be read without content*/}


I don't think it's a big deal to have a span always in the markup but was there some limitation with just doing conditional JSX? Something like:

{this.loading && ( .... )}

jamigibbs · 2025-01-08T19:21:31Z

packages/web-components/src/components/va-button/va-button.tsx

+  @Watch('loading')
+  announceLoadingChange(newValue: boolean, oldValue: boolean) {
+    if (oldValue && !newValue) {
+      var me = this;


Should this variable assignment be let?

jamigibbs · 2025-01-08T19:22:25Z

packages/web-components/src/components/va-button/va-button.tsx

+      this.showCompletedMessage = true;
+      setTimeout(() => {
+        me.showCompletedMessage = false;
+      }, 3000);


I'm not sure I understand the need for a timeout and why this specific time is used. Do you mind clarifying?

The  will only be read by the screen reader if there are changes to the content of the span. This is the reasoning for the span always being rendered, even if the loading prop is false, because if we render it when the prop changes the reader will not count that as a change and "Loading" will never be read.

The same applies to this timeout. If there is not any time between when the completion message is set and when it is set to null, the reader won't detect the change and the message won't be read. Its an arbitrary number to guarantee the "Loading complete" is read.

That makes sense. Thanks for the explanation!

rsmithadhoc

This looked good in my testing, screen readers announced the loading state/complete as expected. Thanks!

rsmithadhoc · 2025-01-09T14:39:46Z

packages/web-components/src/components/va-button/va-button.tsx

+            aria-disabled={ariaDisabled}
+            aria-busy={loading ? 'true' : undefined}
+            aria-label={label}
+            aria-live={loading ? 'polite' : 'off'}


I don't believe we'll need this aria-live on the button itself, as the span will handle the live region. Leaving it in doesn't seem to cause any negative effects, but could lead to repetitive announcements. I tested this code without it and it announced as expected.

va-button: add loading variation

db8bc4b

powellkerry added the minor For a minor Semantic Version change label Jan 7, 2025

powellkerry requested a review from a team as a code owner January 7, 2025 16:03

powellkerry requested review from rsmithadhoc and danbrady January 7, 2025 16:03

Merge branch 'main' into 3460-button-loading

42b42b5

powellkerry commented Jan 7, 2025

View reviewed changes

danbrady reviewed Jan 7, 2025

View reviewed changes

Merge branch 'main' into 3460-button-loading

f190471

jamigibbs approved these changes Jan 8, 2025

View reviewed changes

powellkerry added 3 commits January 8, 2025 13:31

Update var definition to let

998d664

Merge branch 'main' into 3460-button-loading

ef7bff5

update description for loading prop

47be963

rsmithadhoc approved these changes Jan 9, 2025

View reviewed changes

Andrew565 approved these changes Jan 9, 2025

View reviewed changes

powellkerry added 2 commits January 9, 2025 08:55

remove aria-live on button

99f0fe1

fix tests

937adff

powellkerry merged commit 1bbb3a0 into main Jan 9, 2025
8 checks passed

powellkerry deleted the 3460-button-loading branch January 9, 2025 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

va-button: add loading variation #1446

va-button: add loading variation #1446

powellkerry commented Jan 7, 2025 •

edited

Loading

powellkerry Jan 7, 2025

jamigibbs Jan 8, 2025

danbrady left a comment

jamigibbs left a comment

jamigibbs Jan 8, 2025

powellkerry Jan 8, 2025

jamigibbs Jan 8, 2025

powellkerry Jan 8, 2025

jamigibbs Jan 8, 2025

jamigibbs Jan 8, 2025

jamigibbs Jan 8, 2025

jamigibbs Jan 8, 2025

powellkerry Jan 8, 2025

jamigibbs Jan 8, 2025

rsmithadhoc left a comment

rsmithadhoc Jan 9, 2025

va-button: add loading variation #1446

va-button: add loading variation #1446

Conversation

powellkerry commented Jan 7, 2025 • edited Loading

Chromatic

Description

QA Checklist

Screenshots

Acceptance criteria

Definition of done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbrady left a comment

Choose a reason for hiding this comment

jamigibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsmithadhoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powellkerry commented Jan 7, 2025 •

edited

Loading