feat(queryByCurrentValue) #169

eunjae-lee · 2018-12-07T01:49:45Z

What: It adds queryByCurrentValue, getByCurrentValue, queryAllByCurrentValue and getAllByCurrentValue.

Why: getByValue cannot get elements with value property (it only checks DOM value attributes)
(Discussed at #166)

How: Added a set of (get|query)(All)?ByCurrentValue methods.

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table

Feel free to give any feedbacks.

Closes #158

kentcdodds

Thanks! Here are a few comments :)

kentcdodds · 2018-12-07T20:47:09Z

src/queries.js

@@ -356,8 +387,12 @@ export {
  getAllByTitle,
  queryByValue,
  queryAllByValue,
+  queryByCurrentValue,
+  queryAllByCurrentValue,


Could you move both of these down two lines. I kinda like to group things together.

kentcdodds · 2018-12-07T20:50:33Z

src/queries.js

+  const matcher = exact ? matches : fuzzyMatches
+  const matchOpts = {collapseWhitespace, trim}
+  return Array.from(container.querySelectorAll(`input,textarea,select`)).filter(
+    node => matcher(node.value, node, value, matchOpts),


This wont work for selects. Please look at (and maybe just use) the logic in queryAllBySelectText for selects.

kentcdodds · 2018-12-07T20:52:08Z

src/__tests__/element-queries.js

@@ -565,4 +565,19 @@ test('getByText ignores script tags by default', () => {
  expect(getAllByText(/hello/i, {ignore: false})).toHaveLength(3)
 })

+test('get element by its dynamically assigned value', () => {


Could we also have a test for a textarea and a select. Can be in this test or in a new one I don't care which.

eunjae-lee · 2018-12-10T02:19:46Z

Thanks for the review. I made changes regarding your three comments.

alexkrolick · 2018-12-10T05:40:38Z

Looks good! Just need docs and a test to execute line 358 (the error state for queryAllBy...) to keep coverage at 100%.

Also it might be nice to add an integration test for fireEvent.change() with getCurrentValue.

kentcdodds

Looking good, but I do have a question.

kentcdodds · 2018-12-10T05:44:06Z

src/__tests__/element-queries.js

+  expect(queryByCurrentValue('Alaska').id).toEqual('state-select')
+
+  getByTestId('state').value = 'AL'
+  expect(getByCurrentValue('Alabama').id).toEqual('state-select')


I wonder if it's at all confusing that we are expected to set the value to AL but when we use the query we search for Alabama.

I realize that's just how things work (the value for a select has a display value), but I feel like this could be a point of confusion for people. My guess is they'd prefer to select by AL instead which isn't what is shown to the user but feels more intuitive here. I'm not sure of the best solution to this... Maybe allow for either the display value or the actual value?

That makes sense. To end-users Alabama is a value, but AL is the actual value.

However, selectElem.value could be either 1, AL, or whatever user cannot know. In a perspective of writing tests as user would use it, I'd rather choose to restrict it to display value.
What do you think?

Hmm. To really avoid confusion, this could be called getByDisplayValue().

Technically an aria-menu is also compliant with this API: https://w3c.github.io/aria/#menu, https://w3c.github.io/aria/#aria-selected. In the ARIA case there doesn't seem to be any way to have a different "secret form value" vs "display value". EDIT: out of scope

Yeah, I think displayValue is probably a better mental model, though it may be a tiny confusing for input and textarea where the display value is actually the actual value but I think that's alright. Besides, people should normally be getting these elements by their label anyway, so I'm not too concerned about this slightly less-awesome API.

Let's go with getByDisplayValue 👍

eunjae-lee · 2018-12-11T02:39:08Z

@alexkrolick I've added a test to check if it throws.

@kentcdodds I've renamed and updates the README.md. I didn't know how to edit it, so it's kind of a draft. Where do you want to put the section in the document? And how do you want to explain it to users? Take a look at the doc and let me know. Besides, I'm not a native English speaker, so pretty much worried about it 😅

kentcdodds

This is great, just a few small docs changes. Thanks!

kentcdodds · 2018-12-11T03:42:25Z

README.md

@@ -86,6 +86,7 @@ when a real user uses it.
  - [`getByAltText`](#getbyalttext)
  - [`getByTitle`](#getbytitle)
  - [`getByValue`](#getbyvalue)


Let's remove the docs for getByValue and getBySelectText because we want to encourage people to use the getByDisplayValue instead.

kentcdodds · 2018-12-11T03:42:57Z

README.md

+  }): HTMLElement
+```
+
+Returns the element that has the matching display value.


Returns the input, textarea, or select element that has the matching display value.

kentcdodds · 2018-12-11T03:43:22Z

README.md

+// <input type="text" id="lastName" />
+// document.getElementById('lastName').value = 'Norris'
+
+const lastNameInput = getByDisplayValue('Norris')


const lastNameInput = getByDisplayValue(document.body, 'Norris')

kentcdodds · 2018-12-11T03:43:48Z

README.md

+// <textarea id="messageTextArea"></textarea>
+// document.getElementById('messageTextArea').value = 'Hello World'
+
+const messageTextArea = getByDisplayValue('Hello World')


const messageTextArea = getByDisplayValue(document.body, 'Hello World')

kentcdodds · 2018-12-11T03:44:24Z

README.md

+// </select>
+
+const selectElement = getByDisplayName('Alaska')
+console.log(selectElement.id)


Remove the console.log here (it's not necessary and it's nice to be consistent with the others). Then fix it to be:

const selectElement = getByDisplayName(document.body, 'Alaska')

eunjae-lee · 2018-12-11T06:01:40Z

@kentcdodds done! I've used container instead of document.body since I found out other parts of the doc used container.

kentcdodds · 2018-12-11T07:33:07Z

Looks like we're missing some coverage. You can open the coverage report in the browser in coverage/lcov-report to see what's missing.

eunjae-lee · 2018-12-12T01:47:10Z

@kentcdodds coverage 100% done

alexkrolick · 2018-12-12T02:09:16Z

README.md

@@ -337,77 +335,54 @@ content is in an inline script file, then the script tag could be returned.

 If you'd rather disable this behavior, set `ignore` to `false`.

-### `getByAltText`


Why are these doc sections being deleted?

@alexkrolick refer to #169 (comment)

oh my, ..

When I saw that, I somehow thought it was sort of a diff, so I ended up deleting getByAltText, getByTitle, getByValue sections. Let me restore them and get rid of getByValue, getBySelectText sections.

This reverts commit c960865.

eunjae-lee · 2018-12-12T02:31:16Z

Updated the doc. Thanks for pointing it out @alexkrolick

kentcdodds

Sweet! Thanks!

kentcdodds · 2018-12-12T04:49:37Z

🎉 This PR is included in version 3.14.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

eunjae-lee · 2018-12-12T04:51:54Z

Awesome! Thank you :) This merge made my day!!!

eunjae-lee added 2 commits December 7, 2018 10:34

feat(queryByCurrentValue): add get/query by current value

6f596d6

feat(queryByCurrentValue): exports (query|get)AllByCurrentValue

4ae8c95

kentcdodds reviewed Dec 7, 2018

View reviewed changes

eunjae-lee added 3 commits December 10, 2018 10:47

chore: change the order of exports

633028c

fix: add implementation for select element at queryAllByCurrentValue

74fcf1b

chore: add tests for get/queryByCurrentValue

82f1aa0

kentcdodds reviewed Dec 10, 2018

View reviewed changes

eunjae-lee added 4 commits December 10, 2018 15:08

chore: add test for getAllByCurrentValue to throw

eb60b8f

chore: rename xxxByCurrentValue -> xxxByDisplayValue

4a88328

docs: add getByDisplayValue

f941225

docs: add eunjae-lee as a contributor

0ef11c9

kentcdodds reviewed Dec 11, 2018

View reviewed changes

kentcdodds mentioned this pull request Dec 11, 2018

feat(custom normalizer): allow custom control of normalization #172

Merged

4 tasks

eunjae-lee added 2 commits December 11, 2018 15:00

docs: remove getByAltText, getByTitle and getByValue

c960865

docs: update getByDisplayValue

4d3fe95

chore: add test for 100% coverage

661f194

alexkrolick reviewed Dec 12, 2018

View reviewed changes

eunjae-lee added 4 commits December 12, 2018 11:22

docs: remove unnecessary console.log(...)

d93ff6a

Revert "docs: remove getByAltText, getByTitle and getByValue"

1d05d51

This reverts commit c960865.

docs: remove getBySelectText and getByValue

d86856d

docs: update description on getByDisplayName

e5edb46

kentcdodds approved these changes Dec 12, 2018

View reviewed changes

kentcdodds merged commit 9a19474 into testing-library:master Dec 12, 2018

kentcdodds added the released label Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(queryByCurrentValue) #169

feat(queryByCurrentValue) #169

eunjae-lee commented Dec 7, 2018 •

edited by alexkrolick

Loading

kentcdodds left a comment

kentcdodds Dec 7, 2018

kentcdodds Dec 7, 2018

kentcdodds Dec 7, 2018

eunjae-lee commented Dec 10, 2018

alexkrolick commented Dec 10, 2018

kentcdodds left a comment

kentcdodds Dec 10, 2018

eunjae-lee Dec 10, 2018

eunjae-lee Dec 10, 2018

alexkrolick Dec 10, 2018 •

edited

Loading

kentcdodds Dec 10, 2018

eunjae-lee commented Dec 11, 2018

kentcdodds left a comment

kentcdodds Dec 11, 2018

kentcdodds Dec 11, 2018

kentcdodds Dec 11, 2018

kentcdodds Dec 11, 2018

kentcdodds Dec 11, 2018

eunjae-lee commented Dec 11, 2018

kentcdodds commented Dec 11, 2018

eunjae-lee commented Dec 12, 2018

alexkrolick Dec 12, 2018

eunjae-lee Dec 12, 2018

eunjae-lee Dec 12, 2018

eunjae-lee commented Dec 12, 2018

kentcdodds left a comment

kentcdodds commented Dec 12, 2018

eunjae-lee commented Dec 12, 2018

		@@ -337,77 +335,54 @@ content is in an inline script file, then the script tag could be returned.

		If you'd rather disable this behavior, set `ignore` to `false`.

		### `getByAltText`

feat(queryByCurrentValue) #169

feat(queryByCurrentValue) #169

Conversation

eunjae-lee commented Dec 7, 2018 • edited by alexkrolick Loading

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee commented Dec 10, 2018

alexkrolick commented Dec 10, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkrolick Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee commented Dec 11, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee commented Dec 11, 2018

kentcdodds commented Dec 11, 2018

eunjae-lee commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunjae-lee commented Dec 12, 2018

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Dec 12, 2018

eunjae-lee commented Dec 12, 2018

eunjae-lee commented Dec 7, 2018 •

edited by alexkrolick

Loading

alexkrolick Dec 10, 2018 •

edited

Loading