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
feat(input): add item highlight for ionic theme #29395
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This comment was marked as resolved.
This comment was marked as resolved.
@@ -111,6 +111,26 @@ configs({ modes: ['md'] }).forEach(({ title, screenshot, config }) => { | |||
const input = page.locator('ion-input'); | |||
await expect(input).toHaveScreenshot(screenshot(`input-fill-outline`)); | |||
}); | |||
|
|||
test('focus state should not have visual regressions', async ({ page }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a duplicate of this existing test. What's the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm didn't know the outline focus status was being captured already. It didn't catch the border width reducing from 2px
to 1px
(accidental earlier diff).
Let me try a few things locally, such as splitting the images to see if it can detect the change.
I can remove the added test from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to also remove the screenshots that were created from the tests.
Sometimes, it doesn't capture because the change is very minor. You have to delete the screenshot so it can generate a fresh one.
This test case is already handled in the input/test/states test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good and it works to fix the highlight. Makes sense to copy the highlight code from the md
theme. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue number: N/A
What is the current behavior?
The current border implementation causes layout shift.
What is the new behavior?
Does this introduce a breaking change?
Other information