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

fix(item): ensure button focus state on property change #28892

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

BenOsodrac
Copy link
Contributor

@BenOsodrac BenOsodrac commented Jan 26, 2024

Issue number: resolves #28525


What is the current behavior?

When navigated via tab-key, the ion-item is not highlighted correctly after switching from button=false to button=true.

What is the new behavior?

Now, when dynamically changing the the button option to True, there's a @watch callback that will make sure the internal isFocusable @state will be updated.

  • A new unit test was added to item/test. As there was still any unit test for the ion-item, a new files was created - item.spec.tsx

Does this introduce a breaking change?

  • Yes
  • No

Other information

New behavior in runtime:

focusable

@github-actions github-actions bot added the package: core @ionic/core package label Jan 26, 2024
@sean-perkins sean-perkins changed the title [fix](ion-item): update ion-item focus after switching button status fix(item): update focus after switching button status Jan 26, 2024
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Great work! I am able to verify locally that the ion-button is able to receive focus and appropriately styling when the button property is dynamically set.

I am going to reset core/src/components/item/test/buttons/index.html back to origin/main. We have existing screenshots that use the entire page template for the screenshot: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/item/test/buttons/item.e2e.ts#L13 and adding new elements will cause "visual regressions" to the existing screenshot references. Since we are not adding new E2E tests, we don't necessarily need this diff beyond local testing.


import { Item } from '../item';

it('should change focusable option after switching button option status', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I will refactor this when I address the parent comment, for tests we try to nest them under a describe block for the component name.

For example:

describe('item', () => {
  /* existing test */
});

It helps when reviewing the logs locally or in CI to know exactly which component the spec test is responsible for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it! 👍

@sean-perkins sean-perkins changed the title fix(item): update focus after switching button status fix(item): ensure button focus state on property change Jan 26, 2024
@sean-perkins sean-perkins added this pull request to the merge queue Jan 26, 2024
Merged via the queue into ionic-team:main with commit bf7922c Jan 26, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: setting button="true" after load does not make item focusable
2 participants