Skip to content

4361 prevent popover closing on toggle options#4478

Merged
GCHQ-Developer-299 merged 6 commits into
developfrom
4361-prevent-popover-closing-on-toggle-options
Jun 26, 2026
Merged

4361 prevent popover closing on toggle options#4478
GCHQ-Developer-299 merged 6 commits into
developfrom
4361-prevent-popover-closing-on-toggle-options

Conversation

@ad3872

@ad3872 ad3872 commented May 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary of the changes

add a prop on popover menu to prevent closing when any option or toggle options are pressed and added a prop for individual menu options controlling how popover closing behaviour should be on individual menu options

Related issue

#4361

Checklist

General

  • Changes to docs package checked and committed.
  • All acceptance criteria reviewed and met.

Testing

  • Relevant unit tests and visual regression tests added.
  • Visual testing against Figma component specification completed.
  • Playground stories in React Storybook up to date, with any prop changes and additions addressed.
  • Compare performance of modified components against develop using Performance addon in React Storybook.

Resize/zoom behaviour

  • Page can be zoomed to 400% with no loss of content.
  • Screen magnifier used with no issues.
  • Text resized to 200% with no loss of content.

System modes

  • Windows High Contrast mode tested with no loss of content.
  • System light and dark mode tested with no loss of content.
  • Browser support tested (Chrome, Safari, Firefox and Edge).

Testing content extremes

  • Min/max content examples tested with no loss of content or overflow.
  • All prop combinations work without issue.
  • Props/slots can be updated after initial render.

@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

View your branch deployment here: https://mi6.github.io/ic-ui-kit/branches/4361-prevent-popover-closing-on-toggle-options/web-components
View your React branch deployment here: https://mi6.github.io/ic-ui-kit/branches/4361-prevent-popover-closing-on-toggle-options/react

@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 3 times, most recently from ff826de to bbc29f1 Compare May 15, 2026 11:05
@ad3872
ad3872 marked this pull request as ready for review May 15, 2026 11:34
lz405
lz405 reviewed May 18, 2026
View reviewed changes
Comment thread packages/web-components/src/components/ic-popover-menu/ic-popover-menu.tsx Outdated
@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch from bbc29f1 to 4db80b5 Compare May 18, 2026 10:46
@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Cypress visual tests failed.
View the image diff here: https://github.com/mi6/ic-ui-kit/tree/gh-pages/branches/4361-prevent-popover-closing-on-toggle-options/cypress-image-diff-screenshots/diff
View the html report here: https://mi6.github.io/ic-ui-kit/branches/4361-prevent-popover-closing-on-toggle-options/cypress-image-diff-html-report/cypress-image-diff-html-report.html

@lz405

lz405 commented May 20, 2026

Copy link
Copy Markdown
Contributor

The automatically updated docs files containing info about the new props need to be pushed.

@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch from 4db80b5 to 2e02f9b Compare May 28, 2026 10:34
@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 3 times, most recently from 3020612 to 96425c9 Compare May 29, 2026 09:28
@lz405
lz405 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 2 times, most recently from 39803fa to f48965e Compare May 29, 2026 14:18
@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 6 times, most recently from f487074 to b64433a Compare June 1, 2026 14:32
GCHQ-Developer-299
GCHQ-Developer-299 reviewed Jun 2, 2026
View reviewed changes
Comment thread packages/angular-community-supported/src/directives/proxies.ts
@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 3 times, most recently from 63dd3e5 to 45f4200 Compare June 2, 2026 12:02
jd3267
jd3267 previously approved these changes Jun 2, 2026
View reviewed changes
@lz405

lz405 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Its missing one uncommitted change generated by npm run build:all in canary-web-components/src/components.d.ts

@jd3267

jd3267 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Just to check, when clicking the popover menu in the storybook, if you go into actions and then click edit, the menu does close. Is this correct behavior?

@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 4 times, most recently from 45d2bda to ecce901 Compare June 16, 2026 15:29
GCHQ-Developer-299
GCHQ-Developer-299 requested changes Jun 18, 2026
View reviewed changes

@GCHQ-Developer-299 GCHQ-Developer-299 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Functionally everything seems to be working fine, but i've spotted that there's still CSS issues with Destructive variants - seen on the Menu Never Closes story:

Image

@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch from ecce901 to 9fb1570 Compare June 18, 2026 09:26
@ad3872

ad3872 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Functionally everything seems to be working fine, but i've spotted that there's still CSS issues with Destructive variants - seen on the Menu Never Closes story:

Image

Thank you for having a look again! :) I've added that change in now

@ad3872
ad3872 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch from 9fb1570 to a954be1 Compare June 18, 2026 13:18
lz405
lz405 requested changes Jun 22, 2026
View reviewed changes

@lz405 lz405 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Image Image Image

Clicking a menu element that prevents the menu from closing makes the text white on it until another element is clicked. Occurs on both storybooks.

GCHQ-Developer-299
GCHQ-Developer-299 reviewed Jun 24, 2026
View reviewed changes
Comment thread packages/web-components/src/components/ic-menu-item/ic-menu-item.css Outdated
@GCHQ-Developer-299
GCHQ-Developer-299 force-pushed the 4361-prevent-popover-closing-on-toggle-options branch 2 times, most recently from dc6534b to 5fc4b4a Compare June 24, 2026 12:55
@GCHQ-Developer-299

GCHQ-Developer-299 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Image Image Image
Clicking a menu element that prevents the menu from closing makes the text white on it until another element is clicked. Occurs on both storybooks.

I've addressed this issue :)

@GCHQ-Developer-299
GCHQ-Developer-299 requested a review from lz405 June 25, 2026 07:51
@lz405

lz405 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

The issues in the storybook seem to be fixed. However, I'm now getting uncommitted changes to ic-menu-item/readme.md when doing npm run build:all

ad3872 added 6 commits June 26, 2026 12:21
@ad3872 @GCHQ-Developer-299
feat(web-components): add props to prevent closing of popover when op…
b5e2908 
…tions pressed

add a prop on popover menu to prevent closing when any option or toggle options are pressed. added a
prop for individual menu options controlling how popover closing behaviour should be on individual
menu options
@ad3872 @GCHQ-Developer-299
feat(react): add new story and test to show popover menu not closing …
db221e0 
…on click

add a new story to test the new changes where the popover menu does not close
when'close-menu-on-select' is set to false
@ad3872 @GCHQ-Developer-299
docs(docs): docs for CloseMenuOnSelect
f616b4f 
auto generated docs for new prop CoseMenuOnSelect
@ad3872 @GCHQ-Developer-299
docs(canary-docs): canary docs changes
c324e57 
auto generated canary docs changes
@ad3872 @GCHQ-Developer-299
feat(canary-web-components): component docs for closeMenuOnSelect
9d57140 
Auto generated docs for CloseMenuOnSelect for ICMenuItem
@ad3872 @GCHQ-Developer-299
docs(angular-community-supported): angular-community-supported package
df6dccf 
angular-community-supported package changes
@GCHQ-Developer-299

GCHQ-Developer-299 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

The issues in the storybook seem to be fixed. However, I'm now getting uncommitted changes to ic-menu-item/readme.md when doing npm run build:all

Good spot @lz405 - i've run build and also had changes in that readme, so i've added them in :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GCHQ-Developer-847 GCHQ-Developer-847 GCHQ-Developer-847 left review comments
@GCHQ-Developer-299 GCHQ-Developer-299 GCHQ-Developer-299 approved these changes
@lz405 lz405 lz405 approved these changes
@GCHQ-Developer-112 GCHQ-Developer-112 Awaiting requested review from GCHQ-Developer-112
@jd3267 jd3267 Awaiting requested review from jd3267
@GCHQ-Developer-530 GCHQ-Developer-530 Awaiting requested review from GCHQ-Developer-530

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ad3872 @lz405 @jd3267 @GCHQ-Developer-299 @GCHQ-Developer-530 @GCHQ-Developer-847 @GCHQ-Developer-112