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.

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

refactor(modal, panel, popover)!: remove focusId param from setFocus method. #6147

Merged
merged 8 commits into from
Dec 22, 2022

Conversation

driskull
Copy link
Member

@driskull driskull commented Dec 21, 2022

BREAKING CHANGE: Removed focusId parameter from setFocus methods. When the setFocus method is called the first focusable element will be focused.

@driskull driskull requested a review from a team as a code owner December 21, 2022 19:12
@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Dec 21, 2022
# Conflicts:
#	src/components/modal/modal.tsx
#	src/components/popover/popover.tsx
#	src/utils/focusTrapComponent.ts
Copy link
Member

@benelan benelan left a comment

Choose a reason for hiding this comment

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

A couple comments, but LGTM. I think that just leaves pick/value list as the only two left with focusId right?

}

focusFirstTabbable(this);
forceUpdate(this);
Copy link
Member

Choose a reason for hiding this comment

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

this should be forceUpdate(this.el) right?

@@ -552,7 +540,7 @@ export class Popover
renderCloseButton(): VNode {
const { messages, closable } = this;
return closable ? (
<div class={CSS.closeButtonContainer}>
<div class={CSS.closeButtonContainer} key="close-button-container">
Copy link
Member

Choose a reason for hiding this comment

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

Can you use CSS.closeButtonContainer for the key attribute instead of hardcoding it

@@ -575,7 +563,7 @@ export class Popover
) : null;

return headingNode ? (
<div class={CSS.header}>
<div class={CSS.header} key="header">
Copy link
Member

@benelan benelan Dec 21, 2022

Choose a reason for hiding this comment

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

Reuse the string from resources here as well por favor

@driskull driskull merged commit fb241ce into master Dec 22, 2022
@driskull driskull deleted the dris0000/remove-focusid branch December 22, 2022 00:43
@github-actions github-actions bot added this to the 2023 January Priorities milestone Dec 22, 2022
benelan added a commit that referenced this pull request Dec 27, 2022
* master:
  chore(modal): Remove invalid kind values (#6169)
  build: update browserslist db (#6171)
  docs: update component READMEs (#6168)
  refactor(combobox-item)!: remove `toggleSelected` method. (#6162)
  chore(color-picker): add missing message in `en` bundle (#6165)
  refactor(popover)!: remove `toggle` method. (#6161)
  docs: update component READMEs (#6163)
  refactor(modal, panel, popover)!: remove `focusId` param from `setFocus` method. (#6147)
  feat(tab-nav): Add `selectedTitle` property (#6149)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants