Skip to content

Links apathetic to active don't subscribe (#3985) #3986

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

Merged
merged 2 commits into from
Oct 13, 2016
Merged

Links apathetic to active don't subscribe (#3985) #3986

merged 2 commits into from
Oct 13, 2016

Conversation

pshrmn
Copy link
Contributor

@pshrmn pshrmn commented Oct 2, 2016

When a <Link> doesn't need to know about the current location, its <a> does not need to be wrapped with a <LocationSubscriber>.
#3985

@pshrmn
Copy link
Contributor Author

pshrmn commented Oct 2, 2016

I did not include any tests because I am not sure exactly how to test this. There shouldn't be any differences in the DOM and I don't believe that there is a way to access the number of subscribers that the <Broadcast> component has.

Copy link
Member

@timdorr timdorr left a comment

Choose a reason for hiding this comment

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

Needs a test case.

@timdorr
Copy link
Member

timdorr commented Oct 2, 2016

Looks like this would obviate #3896, no?

@pshrmn
Copy link
Contributor Author

pshrmn commented Oct 2, 2016

Added tests. I'm not crazy about the import * as broadcasters from '../locationBroadcast' line, but I needed a target object to spy on.

@ryanflorence
Copy link
Member

tim it first checks if we even care about being active, which is the only
time it matters, i'm still thinking about this one, but i'm inclined to
merge as is
On Sun, Oct 2, 2016 at 11:18 AM Paul Sherman [email protected]
wrote:

Added tests. I'm not crazy about the import * as broadcasters from
'../locationBroadcast' line, but I needed a target object to spy on.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#3986 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGHaN4brFHAW2ZNlyGDaCkLFuMnxEu9ks5qv_V0gaJpZM4KL7eK
.

@timdorr
Copy link
Member

timdorr commented Oct 3, 2016

LGTM. @ryanflorence let me know if you want to add this or not.

@ryanflorence
Copy link
Member

I'm going to merge this, but I intend to refactor it a bit right after, so I'm not going to merge until then.

@pshrmn
Copy link
Contributor Author

pshrmn commented Oct 10, 2016

rebased

@timdorr
Copy link
Member

timdorr commented Oct 11, 2016

Still LGTM 😛

@ryanflorence ryanflorence merged commit 4eee7da into remix-run:v4 Oct 13, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants