Add Disabled State Handling #141

fajar-apri-alaska · 2024-11-07T06:16:40Z

Alaska Airlines Pull Request

This pull request enhances the auro-accordion component by adding support for a disabled state.

Fixes: #123

Summary:

The following changes have been made:

New disabled Property:

Added new disabled property for auro-accordion
Added new disabled property for auro-accordion-group to handle the disabled state of its whole accordion children

Render Method:
Updated the render method to conditionally add the aria-disabled attribute based on the disabled property.
Event Handling:
Modified the toggle method to prevent toggling the accordion content when the component is disabled.
CSS Styles:
Added styles to visually indicate the accordion disabled state by reducing opacity and disabling pointer events.
ARIA Attributes:

Added aria-disabled attribute as conditional attribute
Updated the 'aria-expanded' attribute to be a conditional attribute

Documentation:
Added example and updated the documentation for the new feature for both accordion and accordion-group
Testing:
Added test for the new feature and update 'aria-expanded'-related test

Type of change:

Please delete options that are not relevant.

New capability
Revision of an existing capability
Infrastructure change (automation, etc.)
Other (please elaborate)

Checklist:

My update follows the CONTRIBUTING guidelines of this project
I have performed a self-review of my own update

By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.

Thank you for your submission!

-- Auro Design System Team

Summary by Sourcery

Enhance the auro-accordion component by adding support for a disabled state, including a new 'disabled' property, CSS styling, and documentation updates.

New Features:

Introduce a new 'disabled' property to the auro-accordion component, allowing it to be rendered in a disabled state.

Enhancements:

Update CSS to visually represent the disabled state by reducing opacity and disabling pointer events.

Documentation:

Add documentation and examples for the new 'disabled' attribute in the API documentation.

sourcery-ai · 2024-11-07T06:16:44Z

Reviewer's Guide by Sourcery

This PR implements a disabled state for the auro-accordion component by adding a new disabled property, visual styling, and ARIA attributes. The implementation prevents user interaction when disabled and provides visual feedback through reduced opacity.

Class diagram for AuroAccordion with Disabled State

classDiagram
    class AuroAccordion {
        Boolean expanded
        Boolean emphasis
        Boolean grouped
        Boolean disabled
        String chevron
        String variant
        slot default
        toggle(Event event)
    }
    note for AuroAccordion "Added 'disabled' property and updated 'toggle' method to handle disabled state."

File-Level Changes

Change	Details	Files
Implementation of the disabled state functionality in the accordion component	Added new disabled property to component properties Modified toggle method to prevent interaction when disabled Added aria-disabled attribute to the trigger element Added CSS styles to reduce opacity and disable pointer events	`src/auro-accordion.js` `src/style.scss`
Documentation updates for the new disabled feature	Added new disabled state example with sample code Added documentation for the disabled attribute Updated API documentation with disabled property details	`demo/api.md` `docs/partials/api.md` `apiExamples/disabled.html`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

github-actions · 2024-11-07T06:17:21Z

Surge demo deployment failed! 😭

sourcery-ai

Hey @fajar-apri-alaska - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding unit tests to verify the disabled state behavior, particularly around event handling and accessibility attribute management.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟡 Documentation: 1 issue found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

demo/api.md

demo/api.min.js

demo/index.min.js

jason-capsule42 · 2024-12-09T23:26:21Z

src/style.scss

+:host([disabled]) {
+  .trigger {
+    pointer-events: none;
+    opacity: 0.5;


We don't want to use opacity like this here. Instead we should be setting the text and chevron color to use the disabled token variant.

--ds-color-text-ui-disabled-default --ds-color-icon-ui-disabled-default

updated to use a token reference.

jason-capsule42 · 2024-12-31T18:35:28Z

src/auro-accordion.js

@@ -33,6 +33,7 @@ import tokensCss from "./tokens-css.js";
 * @attr {Boolean} expanded - If set, the accordion is expanded.
 * @attr {Boolean} emphasis - If set, emphasis styles will be applied to the auro-accordions.
 * @attr {Boolean} grouped - Attribute will be set on accordion when it appears in an accordion group.
+ * @attr {Boolean} disabled - If set, the accordion is disabled and have reduced opacity.


This attribute isn't getting listed in the API.md doc and needs to be.

jason-capsule42 · 2024-12-31T18:36:09Z

src/auro-accordion.js

+      disabled: {
+        type: Boolean,
+        reflect: true


You should also be able to disable an entire accordion group without having to do the accordions individually.

sure, I will provide the feature and the documentation for accordion-group also.

jason-capsule42 · 2024-12-31T18:36:44Z

src/style.scss

+  [auro-icon] {
+    color: var(--ds-color-icon-ui-primary-disabled-default, $ds-color-icon-ui-primary-disabled-default);
+  }
+}


the EOF extra line is missing.

jason-capsule42 · 2024-12-31T18:37:54Z

docs/partials/api.md

@@ -163,6 +163,22 @@ Use the `emphasis` attribute to apply border highlights to the `auro-accordion`

 </auro-accordion>

+### disabled


This is in the wrong spot in the doc. Currently it's under the examples for the accordion group and should be moved up in alphabetical order with the regular accordion examples.

Sure, I will put the example above expanded then.

jason-capsule42 · 2024-12-31T18:39:51Z

demo/index.min.js

@@ -742,6 +752,7 @@ class AuroAccordion extends r {
          id="accordionTrigger"
          aria-controls="accordionContent"
          aria-expanded="${this.expanded}"
+          aria-disabled="${this.disabled ? 'true' : 'false'}"


Suggested change

aria-disabled="${this.disabled ? 'true' : 'false'}"

?aria-disabled="${this.disabled}"

You can just conditionally add the attribute based on this.disabled.

It's out of scope of this bug fix but aria-expanded should also actually be done the same way. Feel free to make that change too if you don't mind.

sure, I will add this with additional commit message then.

oh wait, but if we do this, then the aria-* attributes will just basically not available when the value is false.
Is that fine for the screen reader?

Also, we need to update every unit test that checks the attributes, from:
hasAttribute('aria-expanded')).to.equal('false'); -> hasAttribute('aria-expanded')).to.null;
and
hasAttribute('aria-expanded')).to.equal('true'); -> hasAttribute('aria-expanded')).to.not.null;

jason-capsule42 · 2024-12-31T18:42:47Z

src/style.scss

+  [auro-icon] {
+    color: var(--ds-color-icon-ui-primary-disabled-default, $ds-color-icon-ui-primary-disabled-default);
+  }


auro-icon already supports a disabled state. We should use that instead of custom coloring within the accordion.

<auro-icon category="interface" name="pin-trip" disabled></auro-icon>

Oh, I didn't know about this one, I will remember this information also for future work. Thanks.

Hi @jason-capsule42,
After trying to implement this changes, I have concern about this:
The color token that was used for the trigger icon is --ds-auro-accordion-trigger-icon-color) instead of --ds-auro-icon-color

Is this intended? I mean, then we couldn't apply the <auro-icon disabled>, right?

- Add new `disabled` property for auro-accordion - Add new `disabled` property for auro-accordion-group to handle the disabled state of its whole accordion children - Add css styling for the disabled accordion - Add aria-disabled attribute as conditional attribute - Add example and update the documentation for the new feature - Update the 'aria-expanded' attribute to be as conditional attribute - Add test for the new feature and 'aria-expanded'-related test

fajar-apri-alaska · 2025-01-23T04:10:22Z

I have addressed the feedbacks, please re-review @jason-capsule42. Thank you.

fajar-apri-alaska self-assigned this Nov 7, 2024

fajar-apri-alaska requested a review from a team as a code owner November 7, 2024 06:16

fajar-apri-alaska linked an issue Nov 7, 2024 that may be closed by this pull request

auro-accordion: disabled state #123

Open

sourcery-ai bot reviewed Nov 7, 2024

View reviewed changes

demo/api.md Outdated Show resolved Hide resolved

demo/api.min.js Outdated Show resolved Hide resolved

demo/index.min.js Outdated Show resolved Hide resolved

fajar-apri-alaska requested a review from jason-capsule42 November 7, 2024 11:19

jason-capsule42 requested changes Dec 9, 2024

View reviewed changes

fajar-apri-alaska force-pushed the fajarapri/disabledState branch 2 times, most recently from c1360ff to 62ab6c6 Compare December 13, 2024 09:21

fajar-apri-alaska requested a review from jason-capsule42 December 13, 2024 09:42

jason-capsule42 requested changes Dec 31, 2024

View reviewed changes

fajar-apri-alaska force-pushed the fajarapri/disabledState branch from 62ab6c6 to 12008ab Compare January 23, 2025 04:00

fajar-apri-alaska requested a review from jason-capsule42 January 23, 2025 04:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Disabled State Handling #141

Add Disabled State Handling #141

fajar-apri-alaska commented Nov 7, 2024 •

edited

Loading

sourcery-ai bot commented Nov 7, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

github-actions bot commented Nov 7, 2024 •

edited

Loading

sourcery-ai bot left a comment

jason-capsule42 Dec 9, 2024

fajar-apri-alaska Dec 13, 2024 •

edited

Loading

jason-capsule42 Dec 31, 2024

jason-capsule42 Dec 31, 2024

fajar-apri-alaska Jan 23, 2025

jason-capsule42 Dec 31, 2024

jason-capsule42 Dec 31, 2024

fajar-apri-alaska Jan 22, 2025

jason-capsule42 Dec 31, 2024

fajar-apri-alaska Jan 22, 2025

fajar-apri-alaska Jan 22, 2025

jason-capsule42 Dec 31, 2024

fajar-apri-alaska Jan 22, 2025

fajar-apri-alaska Jan 22, 2025

fajar-apri-alaska commented Jan 23, 2025

		@@ -163,6 +163,22 @@ Use the `emphasis` attribute to apply border highlights to the `auro-accordion`

		</auro-accordion>

		### disabled

	aria-disabled="${this.disabled ? 'true' : 'false'}"
	?aria-disabled="${this.disabled}"

Add Disabled State Handling #141

Are you sure you want to change the base?

Add Disabled State Handling #141

Conversation

fajar-apri-alaska commented Nov 7, 2024 • edited Loading

Alaska Airlines Pull Request

Summary:

Type of change:

Checklist:

Summary by Sourcery

sourcery-ai bot commented Nov 7, 2024 • edited Loading

Reviewer's Guide by Sourcery

Class diagram for AuroAccordion with Disabled State

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

github-actions bot commented Nov 7, 2024 • edited Loading

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajar-apri-alaska Dec 13, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fajar-apri-alaska commented Jan 23, 2025

fajar-apri-alaska commented Nov 7, 2024 •

edited

Loading

sourcery-ai bot commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024 •

edited

Loading

fajar-apri-alaska Dec 13, 2024 •

edited

Loading