Skip to content

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

2 participants