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

feat(input): Add component #72

Merged
merged 1 commit into from
Dec 7, 2022
Merged

feat(input): Add component #72

merged 1 commit into from
Dec 7, 2022

Conversation

adamledwards
Copy link
Contributor

@adamledwards adamledwards commented Dec 5, 2022

Adds input component no changes from the gov frontend

test('displays the error message', async ({ page }) => {
await expect(page.getByRole('paragraph')).toHaveText('Error: Enter an event name')
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not to happy with this spec I wanted to use a locator similar to this https://testing-library.com/docs/queries/byrole/#description

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm there's a related open feature request: microsoft/playwright#18332

Copy link
Contributor

@jpveooys jpveooys Dec 6, 2022

Choose a reason for hiding this comment

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

It probably wouldn't be much effort to add a custom toHaveDescription expect matcher to playwright.config.ts and global.d.ts also

Copy link
Contributor Author

@adamledwards adamledwards left a comment

Choose a reason for hiding this comment

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

Ive added the spec that I though was needed, let me know if you think more scenarios should be tested

@adamledwards adamledwards marked this pull request as ready for review December 6, 2022 10:12
@@ -0,0 +1,55 @@
{% from "moduk/components/input/macro.njk" import modukInput %}

{{ modukInput({
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we'll need to wrap these in a <div> because the visual regression test only takes a screenshot of the first element (or maybe it should do something else when there's more than one element).

label: {
text: "What is the name of the event?"
},
id: "event-name",
Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't been doing it so much to date but I was thinking it might be a good idea to keep the ids and names unique in case we embed the examples directly on a page for the component the docs site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

Copy link
Contributor

@jpveooys jpveooys left a comment

Choose a reason for hiding this comment

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

Looks good! Just that comment on the fixed and fluid width examples mainly

@adamledwards adamledwards force-pushed the feature/input branch 2 times, most recently from f3dae84 to aa992cd Compare December 6, 2022 22:33
Copy link
Contributor

@jpveooys jpveooys left a comment

Choose a reason for hiding this comment

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

Nice work!

@@ -30,6 +30,44 @@ expect.extend({
pass: false,
}
},
async toHaveDescription(this: ReturnType<Expect['getState']>, locator: Locator, describeBy: string) {
Copy link
Contributor

@jpveooys jpveooys Dec 7, 2022

Choose a reason for hiding this comment

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

Does expected or description make more sense than describeBy?

Suggested change
async toHaveDescription(this: ReturnType<Expect['getState']>, locator: Locator, describeBy: string) {
async toHaveDescription(this: ReturnType<Expect['getState']>, locator: Locator, expected: string) {


const count = await receivedResult.count()
for (let i = 0; i < count; i += 1) {
receivedTextPromises.push(receivedResult.nth(i).textContent())
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to call .trim() on the text content (only a cosmetic thing).

promise: this.promise,
})
}\n
Received: ${this.utils.printReceived(receivedText.map((text) => text?.trim()).join('\n'))}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpveooys I had to run the trim through a loop here because the loop returns a promise and we have a eslint lint rule that forbids using await inside a loop

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks tidy enough 👍

@adamledwards adamledwards merged commit d1b4503 into main Dec 7, 2022
@adamledwards adamledwards deleted the feature/input branch December 7, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants