feat(visible): Add visible() method on Wrapper (#327) #335

Toilal · 2018-01-08T15:48:02Z

Ready ! :)

eddyerburgh · 2018-01-08T17:57:10Z

Thanks for the PR 😀

I don't think we should add a parent method as part of this PR, or at all —#65

You should use the element parentNode/parentElement properties to traverse the parent tree.

Like you found, most vnodes have an undefined parent. Functional vnodes use the parent property.

Toilal · 2018-01-09T08:26:46Z

I just removed parent(), implement visible() using HTMLElement, and added tests. It seems to work now :p.

visible() is also consistent with exists() (if exists() == false then visible() == false)

eddyerburgh · 2018-01-09T19:09:50Z

package.json

@@ -10,7 +10,7 @@
  ],
  "scripts": {
    "build": "node build/build.js",
-    "build:test": "NODE_ENV=test node build/build.js",
+    "build:test": "cross-env NODE_ENV=test node build/build.js",


Thanks, good spot

eddyerburgh · 2018-01-09T19:12:03Z

src/wrappers/wrapper.js

+   */
+  visible (): boolean {
+    if (!this.exists()) {
+      console.log('NOT EXISTS')


We should throw an error here using the throwError function:
throwError('cannot call visible on a wrapper without an element')

eddyerburgh · 2018-01-09T19:13:28Z

src/wrappers/wrapper.js

+   * Utility to check wrapper is visible. Returns false if a parent element has display: none or visibility: hidden style.
+   */
+  visible (): boolean {
+    if (!this.exists()) {


You should check for an element here instead of exists—if(!this.element). That's how we check for an element in other tests (I think this might solve the flow error)

eddyerburgh · 2018-01-09T19:13:49Z

src/wrappers/wrapper.js

+      }
+      element = element.parentElement
+    }
+    console.log('no parent')


We should remove these console logs

eddyerburgh

Looks good, just a couple of changes. Also we need the flow error to be fixed

eddyerburgh · 2018-01-12T06:22:09Z

Are you able to make these changes @Toilal ?

Toilal · 2018-01-12T06:51:43Z

Oops, I just forget this one, i'll do ASAP

Toilal · 2018-01-12T07:34:30Z

Done

38elements · 2018-01-14T14:21:08Z

I think it is necessary to add this to the document.

eddyerburgh · 2018-01-14T14:43:07Z

Yes this should be added to the documentation before merging

…

On 14 Jan 2018 14:21, "38elements" ***@***.***> wrote: I think it is necessary to add this to the document. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#335 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMlbW-sJq-X7_hmFySwR4CiB9VkiFl1Lks5tKg1XgaJpZM4RWjoh> .

eddyerburgh · 2018-01-15T06:23:27Z

@Toilal Can you add a page on visible to docs/en/wrapper/api/visible.md, docs/en/api/wrapper-array/visible.md, and add a links to both pages in docs/en/SUMMARY.

Toilal · 2018-01-15T13:23:19Z

Rebased and docs added.

Toilal · 2018-01-16T08:03:56Z

Can you restard the build ?

Alex-Sokolov · 2018-01-16T08:05:30Z

docs/en/api/wrapper/visible.md

@@ -0,0 +1,21 @@
+# exists()


title missed

eddyerburgh · 2018-01-16T08:25:43Z

docs/en/SUMMARY.md

@@ -63,6 +64,7 @@
    * [setProps](api/wrapper-array/setProps.md)
    * [trigger](api/wrapper-array/trigger.md)
    * [update](api/wrapper-array/update.md)
+    * [visible](api/wrapper/visible.md)


This should be wrapper-array

I've made the same as exists() with use the same page.

I didn't realize exists uses the same page, this is an error with the exists page.

The wrapper-array pages have different examples that use a wrapper-array

Should I fix exists page too ? Or another pull request ?

eddyerburgh · 2018-01-16T08:26:37Z

docs/en/api/wrapper/visible.md

+Assert `Wrapper` or `WrapperArray` is visible.
+
+Returns false if an ancestor element has `display: none` or `visibility: hidden` style.
+


We should add that this is useful for testing whether v-show is working correctly

eddyerburgh · 2018-01-16T08:27:19Z

docs/en/api/wrapper/visible.md

+
+const wrapper = mount(Foo)
+expect(wrapper.visible()).toBe(true)
+expect(wrapper.find('is-not-visible').visible()).toBe(false)


is-not-visible should be a class selector, not a tag selector

eddyerburgh · 2018-01-16T08:28:30Z

src/wrappers/error-wrapper.js

@@ -36,6 +36,10 @@ export default class ErrorWrapper implements BaseWrapper {
    return false
  }

+  visible (): boolean {
+    return false


This should throw an error, like the other methods.

Only exists returns false, everything else throws an error to indicate that find was not succesful

Don't you think it should match the behavior of exists() method ? Having this return an error would make assertions on non-visible elements more verbose.

No, I think it should throw an error. exists checks if find is successful, so it makes sense to return false. If find('div') is not successful, exists should return false.

Every other method checks something on the Wrapper element/ instance.

We need to make a distinction between a node being visible and a node being present.

eddyerburgh · 2018-01-16T08:28:51Z

test/unit/specs/mount/Wrapper/visible.spec.js

+    wrapper.update()
+    expect(wrapper.find('.child.ready').visible()).to.equal(true)
+  })
+})


Nice tests 👌

Toilal · 2018-01-16T15:06:28Z

I just squashed everything and make visible() method fails for non-existent elements. I think it's know OK :p

eddyerburgh · 2018-01-16T17:26:25Z

docs/en/api/wrapper/visible.md

+const wrapper = mount(Foo)
+expect(wrapper.visible()).toBe(true)
+expect(wrapper.find('.is-not-visible').visible()).toBe(false)
+expect(wrapper.findAll('div').visible()).toBe(true)


Can you remove the findAll examples please

eddyerburgh · 2018-01-20T06:45:19Z

Thanks @Toilal 🙂

Toilal force-pushed the visible branch from 3c55dfc to 027c615 Compare January 9, 2018 08:24

Toilal force-pushed the visible branch 3 times, most recently from 387c59e to e7ee4da Compare January 9, 2018 08:46

eddyerburgh reviewed Jan 9, 2018

View reviewed changes

eddyerburgh requested changes Jan 9, 2018

View reviewed changes

Toilal force-pushed the visible branch from e7ee4da to ed979c8 Compare January 12, 2018 07:19

Toilal changed the title ~~feat(visible): Add visible() and parent() methods on Wrapper (#327)~~ feat(visible): Add visible() method on Wrapper (#327) Jan 13, 2018

Toilal force-pushed the visible branch from e344b45 to 52052c7 Compare January 15, 2018 13:23

Alex-Sokolov reviewed Jan 16, 2018

View reviewed changes

Toilal force-pushed the visible branch from 52052c7 to d1ed645 Compare January 16, 2018 08:22

eddyerburgh reviewed Jan 16, 2018

View reviewed changes

Toilal force-pushed the visible branch from d1ed645 to aff4743 Compare January 16, 2018 08:34

Toilal force-pushed the visible branch 2 times, most recently from dbce8b6 to d214733 Compare January 16, 2018 15:04

Toilal force-pushed the visible branch 2 times, most recently from 42a153a to 113aff8 Compare January 16, 2018 15:40

eddyerburgh reviewed Jan 16, 2018

View reviewed changes

feat(visible): Add visible() method on Wrapper (vuejs#327)

ea7e2d6

Toilal force-pushed the visible branch from 113aff8 to ea7e2d6 Compare January 17, 2018 07:11

eddyerburgh approved these changes Jan 20, 2018

View reviewed changes

eddyerburgh merged commit 445a72d into vuejs:dev Jan 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(visible): Add visible() method on Wrapper (#327) #335

feat(visible): Add visible() method on Wrapper (#327) #335

Toilal commented Jan 8, 2018 •

edited

Loading

eddyerburgh commented Jan 8, 2018 •

edited

Loading

Toilal commented Jan 9, 2018 •

edited

Loading

eddyerburgh Jan 9, 2018

eddyerburgh Jan 9, 2018

eddyerburgh Jan 9, 2018

eddyerburgh Jan 9, 2018

eddyerburgh left a comment

eddyerburgh commented Jan 12, 2018

Toilal commented Jan 12, 2018

Toilal commented Jan 12, 2018

38elements commented Jan 14, 2018

eddyerburgh commented Jan 14, 2018 via email

eddyerburgh commented Jan 15, 2018

Toilal commented Jan 15, 2018

Toilal commented Jan 16, 2018

Alex-Sokolov Jan 16, 2018

Toilal Jan 16, 2018

Toilal Jan 16, 2018

eddyerburgh Jan 16, 2018

Toilal Jan 16, 2018

eddyerburgh Jan 16, 2018

Toilal Jan 16, 2018

eddyerburgh Jan 16, 2018 •

edited

Loading

Toilal Jan 16, 2018

eddyerburgh Jan 16, 2018 •

edited

Loading

eddyerburgh Jan 16, 2018

Toilal Jan 16, 2018

eddyerburgh Jan 16, 2018

eddyerburgh Jan 16, 2018

Toilal commented Jan 16, 2018

eddyerburgh Jan 16, 2018

Toilal Jan 17, 2018

eddyerburgh commented Jan 20, 2018

		Assert `Wrapper` or `WrapperArray` is visible.

		Returns false if an ancestor element has `display: none` or `visibility: hidden` style.

feat(visible): Add visible() method on Wrapper (#327) #335

feat(visible): Add visible() method on Wrapper (#327) #335

Conversation

Toilal commented Jan 8, 2018 • edited Loading

eddyerburgh commented Jan 8, 2018 • edited Loading

Toilal commented Jan 9, 2018 • edited Loading

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

eddyerburgh left a comment

Choose a reason for hiding this comment

eddyerburgh commented Jan 12, 2018

Toilal commented Jan 12, 2018

Toilal commented Jan 12, 2018

38elements commented Jan 14, 2018

eddyerburgh commented Jan 14, 2018 via email

eddyerburgh commented Jan 15, 2018

Toilal commented Jan 15, 2018

Toilal commented Jan 16, 2018

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

Choose a reason for hiding this comment

eddyerburgh Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyerburgh Jan 16, 2018 • edited Loading

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

Toilal commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyerburgh commented Jan 20, 2018

Toilal commented Jan 8, 2018 •

edited

Loading

eddyerburgh commented Jan 8, 2018 •

edited

Loading

Toilal commented Jan 9, 2018 •

edited

Loading

eddyerburgh Jan 16, 2018 •

edited

Loading

eddyerburgh Jan 16, 2018 •

edited

Loading