Skip to content

Conversation

georgylobko
Copy link
Member

@georgylobko georgylobko commented Sep 2, 2025

Description

This PR adds a new API for declaring header actions in global panels registered through the plugin API. The goal is to make it easier to add actions to panel headers. Currently, teams have to create custom buttons
with absolute positioning, which causes accessibility issues. This PR fixes that by using the existing ButtonGroup component instead.

Related links, issue #, if available: n/a

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 97.13%. Comparing base (ce7b93a) to head (1bc402d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/app-layout/runtime-drawer/index.tsx 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3824      +/-   ##
==========================================
- Coverage   97.14%   97.13%   -0.02%     
==========================================
  Files         844      844              
  Lines       24515    24549      +34     
  Branches     8645     8658      +13     
==========================================
+ Hits        23815    23845      +30     
- Misses        693      697       +4     
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

expect(onToggle).toHaveBeenCalledWith({ isOpen: false, initiatedByUserAction: true });
});

(type === 'global-ai' ? test : test.skip)(
Copy link
Member

Choose a reason for hiding this comment

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

I started a new file for this new API: https://github.com/cloudscape-design/components/blob/main/src/app-layout/__tests__/runtime-drawers-widgetized.test.tsx

It is better to add new exclusive features there

Copy link
Member Author

Choose a reason for hiding this comment

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

this test is no longer specific for the ai drawer - it now applies for to both global and global-ai

if (!root) {
return;
}
refs.close = { current: root.querySelector('[data-itemid="close"]') as unknown as Focusable };
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to any suggestion to make it better

Copy link
Member

Choose a reason for hiding this comment

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

Ok to merge as a temporary solution.

But let's ask in the P&C channel for help. Most likely we should expose a new package-private API to assign refs on button group items

Copy link
Member

@just-boris just-boris left a comment

Choose a reason for hiding this comment

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

All comments ⛳ are merge blocking, the rest are follow ups

id: 'add',
iconName: 'add-plus',
text: 'Add',
popoverFeedback: <StatusIndicator type="success">Message copied</StatusIndicator>,
Copy link
Member

Choose a reason for hiding this comment

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

⛳ This is not going to work in the real world when the React instance used in the widget and version used in app layout are different

Perhaps we should omit all slot properties from this API (or print a warning they are not supported in this mode)

Copy link
Member Author

Choose a reason for hiding this comment

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

Printed a warning for unsupported props

Copy link
Member

Choose a reason for hiding this comment

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

We need to remove these instances, because having them with a warning may confuse if this use-case is supported or not (it is not supported)

const animationDisabled =
(activeGlobalDrawer?.defaultActive && !drawersOpenQueue.includes(activeGlobalDrawer.id)) ||
(wasExpanded && !isExpanded);
let drawerActions: ReadonlyArray<ButtonGroupProps.InternalItemOrGroup> = [
Copy link
Member

Choose a reason for hiding this comment

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

⛳ Is it the same code as above?

This needs to be deduplicated

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it might look similar, but it's not the same. global drawer and left global drawer have slightly different set of props

if (!root) {
return;
}
refs.close = { current: root.querySelector('[data-itemid="close"]') as unknown as Focusable };
Copy link
Member

Choose a reason for hiding this comment

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

Ok to merge as a temporary solution.

But let's ask in the P&C channel for help. Most likely we should expose a new package-private API to assign refs on button group items

popoverFeedback?: React.ReactNode;
}

export interface InternalIconButton extends IconButton {
Copy link
Member

Choose a reason for hiding this comment

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

⛳ This lives inside ButtonGroupProps namespace which is public.

You need to move all private additions outside this namespace to avoid exporting to public

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

@georgylobko georgylobko force-pushed the feat/ai-drawer-header-actions branch from 177de3f to 1bc402d Compare September 9, 2025 13:32
@georgylobko georgylobko added this pull request to the merge queue Sep 9, 2025
Merged via the queue into main with commit 979d270 Sep 9, 2025
41 of 42 checks passed
@georgylobko georgylobko deleted the feat/ai-drawer-header-actions branch September 9, 2025 14:46
at-susie pushed a commit that referenced this pull request Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants