Feature/multiple portals in one target #80

eschalks · 2017-10-03T20:23:44Z

Adds a multiple prop to the portal-target that, when true, will display all portals with the same target instead of just the first one.

Haven't written any tests yet, but I'll gladly add some if there's a real chance this PR might be accepted.

… true.

…om sometimes duplicating elements.

LinusBorg · 2017-10-03T20:32:20Z

Hey @eschalks

Many thanks for this! As you can see in #76 I tried another approach, but that was a dead end, so this is a welcome starting point, looks good in first sight.

I'll take a closer look hopefully tomorrow night.

I was thinking about doing something similar but was hesitant to use an array because I do't like looking up stuff in arrays for performance reasons - but seeing this I think, this portal concept isn't meant to be used with hundeds of sources all sending to the same destination, so that should be fine.

I'll report back ASAP, then you can add some more tests. If I find anything off, I will push directly to your branch, hope that's ok.

LinusBorg · 2017-10-05T14:44:16Z

src/components/wormhole.js

+
+    const currentIndex = this.getTransportIndex(transport)
+    if (currentIndex === -1) {
+      this.transports[to].push(transport)


I think we should maybe use .shift instead, so that for Targets with multiple=false, the latest source is displayed at all times.

I definitely agree that I'd expect the last one to be the one to appear if multiple=false. But if we use shift the portals will appear in reverse order if multiple=true.

Maybe we could change the last line of ownTransports (in portal-target) to this instead:

return transports.length === 0 ? [] : [transports[transports.length - 1]]

For an additional feature idea:
Maybe it would be nice to add a :order prop to portals so users can decide in which order the portals should appear when multiple is true.
(Although I haven't actually thought of a use case yet, since I'm currently only planning to use multiple for moving all my modals to the end of the DOM.)

Yeah, goping for the last element is even better I think. Will review in more depth this weekend.

Nice work, many thanks! :)

LinusBorg

necessary change in emitChange

    emitChange (newTransport, oldTransport) {
       this.$emit('change',
-         { ...newTransport },
-         { ...oldTransport }
+         [...newTransport],
+         [...oldTransport]
       )
      },

LinusBorg · 2017-10-08T16:24:41Z

src/components/portal-target.js

      firstRender: true,
    }
  },
  mounted () {
    if (!this.transports[this.name]) {
-      this.$set(this.transports, this.name, undefined)
+      this.$set(this.transports, this.name, [])


Unrelated to the feature, but I think this whole if block (but not the rest) should be in created() rather than mounted so it's applied before first render.

LinusBorg · 2017-10-08T16:25:55Z

test/unit/portal-target.spec.js

@@ -1,11 +1,12 @@
-import { expect, td } from './helpers'


Please respect the code style. This repo has space paddings for inline object notation.

This applies to all changed files, I won't repeat it everywhere in the review for brevity.

LinusBorg · 2017-10-08T16:32:21Z

test/unit/portal-target.spec.js

-      to: 'target',
-      passengers: vNode,
-    })
+    Vue.set(transports, 'target', [


This change is correct and necessary, but was not applied to all test cases where Vue.set was used.

LinusBorg · 2017-10-08T16:33:33Z

test/unit/portal-target.spec.js

@@ -144,7 +152,8 @@ describe('PortalTarget', function () {
    })
    return vm.$nextTick().then(() => {
      td.verify(spy({
-        to: 'target', from: 'source', passengers: vNodes },
+          to: 'target', from: 'source', passengers: vNodes


this should now receive arrays. spy([ ... ], [ ... ]).

...when the emitChange method in PortalTarget has been changed as described further above.

eschalks · 2017-10-10T18:10:35Z

Performed all the changes you requested in the review.

LinusBorg · 2017-10-11T08:02:52Z

Thanks, will take a look tonight and merge if everything passes.

LinusBorg · 2017-10-17T08:53:54Z

Hey, sorry for leaving you hanging a bit - my private life is eating up my free time slots I would need to thoroughly look at this and merge it.

That situation should improve next week, and I plan to dedicated sir time to this project at next week's weekend.

eschalks · 2017-10-17T16:53:07Z

Don't worry about it. I'm currently using this branch in the one project where I need this functionality, so there's no need to hurry on my account.

Hope your personal situation works out well!

LinusBorg · 2017-10-17T18:28:21Z

Sure, it's all fine - weddings to attend and contribute to, this kind of stuff. so nothing to worry about, but these things eat time. :D

LinusBorg · 2017-11-19T09:21:13Z

Thank you very much from this. I will try and add some more tests for this and add docs, then prepare a release.

eschalks added 7 commits October 3, 2017 20:24

Setup initial example page

8df6c84

Collect passengers from all portals with the same "to" if multiple is…

6e077b5

… true.

Updated example in order to develop wormhole closing.

178bf9d

Remove the appropriate transport from the wormhole when closing.

3a3ce5f

Fixed wormhole tests.

ad4e795

Fixed PortalTarget tests

766073d

When multiple is enabled, wrap transports into Tags to prevent Vue fr…

4a5cc90

…om sometimes duplicating elements.

LinusBorg self-requested a review October 3, 2017 20:32

LinusBorg added the feature label Oct 3, 2017

LinusBorg self-assigned this Oct 3, 2017

Update existing transport when from is already opened.

ed2c836

LinusBorg reviewed Oct 5, 2017

View reviewed changes

eschalks added 3 commits October 5, 2017 21:44

Sort portals by order

51a0943

Preventing child duplication when toggling multiple.

c8e39e2

Ensure unique wrapper key in a less random way

2ce93c5

LinusBorg requested changes Oct 8, 2017

View reviewed changes

LinusBorg mentioned this pull request Oct 9, 2017

PortalTargetGroup - A component to show multiple sources at the same time #76

Closed

eschalks added 7 commits October 10, 2017 19:31

Change event now emits the change in arrays when multiple is true.

2821960

Spaces in inline objects

436630e

Portal target now ensures own transport array exists in created hook.

4f2ff2d

Fix spec curly braces.

5a2a6fc

Added test for change when multiple is true.

5a3f41e

lint fixes in test files

11bc716

All PortalTarget tests now Vue.set an array in transports.

cc1cf5f

LinusBorg mentioned this pull request Oct 12, 2017

Component scope #81

Closed

LinusBorg added the PR: Review required label Nov 7, 2017

LinusBorg added this to the 1.2.0 milestone Nov 7, 2017

LinusBorg approved these changes Nov 19, 2017

View reviewed changes

LinusBorg merged commit b42bbbe into LinusBorg:develop Nov 19, 2017

LinusBorg mentioned this pull request Dec 17, 2017

Error in render: "ReferenceError: 'Symbol' is undefined" when run on IE11 #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/multiple portals in one target #80

Feature/multiple portals in one target #80

eschalks commented Oct 3, 2017

LinusBorg commented Oct 3, 2017

LinusBorg Oct 5, 2017

eschalks Oct 5, 2017 •

edited

Loading

LinusBorg Oct 6, 2017

LinusBorg left a comment •

edited

Loading

LinusBorg Oct 8, 2017

LinusBorg Oct 8, 2017

LinusBorg Oct 8, 2017

LinusBorg Oct 8, 2017 •

edited

Loading

eschalks commented Oct 10, 2017

LinusBorg commented Oct 11, 2017

LinusBorg commented Oct 17, 2017

eschalks commented Oct 17, 2017

LinusBorg commented Oct 17, 2017

LinusBorg commented Nov 19, 2017 •

edited

Loading

Feature/multiple portals in one target #80

Feature/multiple portals in one target #80

Conversation

eschalks commented Oct 3, 2017

LinusBorg commented Oct 3, 2017

LinusBorg Oct 5, 2017

Choose a reason for hiding this comment

eschalks Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

LinusBorg Oct 6, 2017

Choose a reason for hiding this comment

LinusBorg left a comment • edited Loading

Choose a reason for hiding this comment

LinusBorg Oct 8, 2017

Choose a reason for hiding this comment

LinusBorg Oct 8, 2017

Choose a reason for hiding this comment

LinusBorg Oct 8, 2017

Choose a reason for hiding this comment

LinusBorg Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

eschalks commented Oct 10, 2017

LinusBorg commented Oct 11, 2017

LinusBorg commented Oct 17, 2017

eschalks commented Oct 17, 2017

LinusBorg commented Oct 17, 2017

LinusBorg commented Nov 19, 2017 • edited Loading

eschalks Oct 5, 2017 •

edited

Loading

LinusBorg left a comment •

edited

Loading

LinusBorg Oct 8, 2017 •

edited

Loading

LinusBorg commented Nov 19, 2017 •

edited

Loading