-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Autocomplete test helpers #248
Conversation
🦋 Changeset detectedLatest commit: 52ded27 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
a95bfb8
to
73ab9c3
Compare
Preview URLsEnv: preview |
4a2cc71
to
b37fb5f
Compare
@@ -19,6 +19,10 @@ Toucan provides a set of accessible and reusable components that make it easy to | |||
|
|||
The `core` package contains the Toucan-styled Ember components. The `form` package allows users to build forms using [`ember-headless-form`](https://github.com/CrowdStrike/ember-headless-form) with the `core` components. | |||
|
|||
## Usage |
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.
Bonus. Moved to the top. Happy to revert! But I'm guessing this what most people will be looking for when they visit the repository.
onMouseover=(fn this.onOptionMouseover index) | ||
popoverId=this.popoverId | ||
index=index | ||
<div id="autocomplete"> |
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.
No changes other than I wrapped everything in <div id="autocomplete">
. This gives test helpers a parent (closest
) to select, which lets them select various children.
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.
Mostly just wondering, but any reason to use id
over data-
attribute? closest
should work with those as well, right?
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.
Oh, for sure. I went back and forth on it. I'll spare you my reasoning and switch it to data-autocomplete
.
As an aside, if we ever rebuild these components, I'd favor a a slightly different form (data-test="autocomplete"
) throughout so it's clear what these selectors are for when perusing the templates.
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'll spare you my reasoning and switch it to data-autocomplete.
Done
@@ -4,12 +4,12 @@ import { module, test } from 'qunit'; | |||
import Button from '@crowdstrike/ember-toucan-core/components/button'; | |||
import { setupRenderingTest } from 'test-app/tests/helpers'; | |||
|
|||
import { Button as TestButton } from '@crowdstrike/ember-toucan-core/test-support'; | |||
import { ButtonPageObject } from '@crowdstrike/ember-toucan-core/test-support'; |
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.
Updated to match Autocomplete and to be more descriptive.
@@ -9,7 +9,7 @@ module('Integration | Component | Fields | Autocomplete', function (hooks) { | |||
|
|||
test('it renders', async function (assert) { | |||
await render(<template> | |||
<Autocomplete @noResultsText="No results" @label="Label" data-autocomplete /> | |||
<Autocomplete @noResultsText="No results" @label="Label" data-input /> |
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.
Changed to data-input
throughout so it's clearer that it's the <input />
and not the component's wrapping element.
|
||
export class AutocompletePageObject extends PageObject { | ||
get active() { | ||
return this.element |
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.
We spread attributes
on the <input />
, which seems right to me. However, it means we need to first get ahold of the parent element (#autocomplete
) before we can select children.
@@ -96,12 +96,12 @@ export default class ToucanCoreAutocompleteOptionComponent extends Component<Tou | |||
<template> | |||
{{! template-lint-disable require-presentational-children }} | |||
<li | |||
aria-current={{if @isActive "true" "false"}} |
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.
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.
Replaces data-active
(below).
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.
Oh very nice!
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.
Shall we add a changeset as new test-helpers will be exposed?
@@ -96,12 +96,12 @@ export default class ToucanCoreAutocompleteOptionComponent extends Component<Tou | |||
<template> | |||
{{! template-lint-disable require-presentational-children }} | |||
<li | |||
aria-current={{if @isActive "true" "false"}} |
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.
Oh very nice!
onMouseover=(fn this.onOptionMouseover index) | ||
popoverId=this.popoverId | ||
index=index | ||
<div id="autocomplete"> |
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.
Mostly just wondering, but any reason to use id
over data-
attribute? closest
should work with those as well, right?
@@ -2,7 +2,7 @@ import { click } from '@ember/test-helpers'; | |||
|
|||
import { PageObject } from 'fractal-page-object'; | |||
|
|||
export class Button extends PageObject { | |||
export class ButtonPageObject extends PageObject { |
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.
Due to an export name change, should we consider a breaking-change changeset? I'd be okay making it a patch
, as I'm not sure anyone is using this directly. As long as it's documented in the release notes 👍
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'm cool with that.
Though I tend to wait to add a changeset until after PR review in case there's some discussion. I'll make sure to add one after approval.
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.
Done.
b37fb5f
to
52ded27
Compare
🚀 Description
🔬 How to Test
We're good if the build is green.