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

Recommend a channel with a URL parameter #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danpaz
Copy link

@danpaz danpaz commented Oct 19, 2017

Resolves #7 - Trying out @caseywatts's idea of conditionally showing a 4th step to point invitees to a specific channel. Add ?recommendChannel=hi to the url. Is this what you had in mind @caseywatts?

image

There are probably some edge cases to consider. For example this doesn't scrub the query param, although ?recommendChannel=<script>alert()</script> properly escapes 😅.

src/index.html Outdated
return(false);
}

var containerElem = document.getElementById("recommend-channel-container");
Copy link
Contributor

Choose a reason for hiding this comment

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

at a glance, I'm not sure what containerElem is - sounds like it might be for the whole page?
maybe it could be called channelContainerElem

src/index.html Outdated

var containerElem = document.getElementById("recommend-channel-container");
var channelElem = document.getElementById("recommend-channel");
var channel = getQueryVariable("recommendChannel");
Copy link
Contributor

Choose a reason for hiding this comment

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

let's call it recommendedChannel instead of recommendChannel?

I think that'll make more sense from the referrer's side (like from dccodecoffee.com)

@caseywatts
Copy link
Contributor

awesome! I have two more ideas for this.

1: We could make this tile link to the channel via
https://dctech.slack.com/messages/dccodecoffee

2: Text change suggestion:
Remove Check Out, leaving just the hashtagged channel name.

@caseywatts
Copy link
Contributor

one more:

3: maybe we want to show the card saying "join channels" even if we don't recommend one? It wouldn't list a recommended channel or link to a specific channel (it could link to dctech.slack.com). (I'm only 50% on wanting this)

@caseywatts
Copy link
Contributor

once this is in we can replace two buttons with just one on so many sites 🎉
image

can become one button that says:

DCTech Slack
#dccodecoffee

@danpaz
Copy link
Author

danpaz commented Dec 3, 2017

We discussed IRL:

  • 4th box only if query param is there
  • text should say Join Channel \

@danpaz
Copy link
Author

danpaz commented Dec 3, 2017

@caseywatts Ready for another look

@caseywatts
Copy link
Contributor

Code looks good!

Let's test these cases before merging:

  • url with no recommended channel
  • url with valid recommended channel
  • url with invalid recommended channel
  • url with recommended channel but no value set to it

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