-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
Adding call info to the alert bar Encapsulated logic in function Hide alert bar Added call link
@ipfs/infra Having trouble figuring out this Jenkins error- not sure what my next steps should be. |
Who do we need to loop in to review this? |
@@ -0,0 +1,16 @@ | |||
var $ = require('jquery') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not import jquery just to create a pop up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviddias Jquery wouldn't be my first choice either, but it is already used in ipfs.io site. https://github.com/ipfs/website/blob/master/js/lib/popup.js https://github.com/ipfs/website/blob/master/js/lib/blog-feed.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, we’re already using it on two other pages. if someone wants to send a PR to remove it’s use throughout the site, fine, but I don’t see why we would hold up new contributions since it’s not actually adding a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jquery wouldn't be my first choice either, but it is already used in ipfs.io site.
Understood. Although I would argue that the best way to default not be the first choice anymore is to move away from it rather than adding it in more places.
@pkafei I'm wondering how effective this popup banner would be. Did you study the analytics on the webpage to see if this is something that folks have open throughout the entire day or if it is common for folks to have the website open before the call? A nice page presenting the Working Groups and Calls schedule could have more success as it would be a static resource that people could link to. |
We don’t really need analytics to tell us that people will see a message at the very top of the website. The reason it only appears during certain hours isn’t because we think people have the site up the entire time but because it’s not all that effective to try and get people to participate in the call when it’s nowhere near the time of the call. The purpose of this bar is to drive participation in the weekly call. Analytics can tell us how many people it will address but the analytics can’t tell us if increasing participation in the call is worth above-the-fold website real estate, that’s a question we have to answer based on our own priorities. We’re working under the assumption that it is a priority, but if it’s not then we should have that discussion.
We should definitely do this, but I don’t see how these are mutually exclusive. It was called out as a strategic goal back in Q4 to drive participation in this call specifically. Adding this to a list of calls for all of our working groups isn’t a great way to achieve that specific goal, although it’s a good idea and we should definitely do it. If we’re going to lump any effort to drive participation in this call into a goal around participation in all of the calls then why do we have KPI’s and OKR’s associated with this call specifically, which actually pre-date @pkafei even working on this? |
js/lib/alert-bar.js
Outdated
var dayOfWeek = new Date().getDay() | ||
var reminder = $('.alert-bar') | ||
var announcement = $('.lh-title').append('<span>The IPFS community call will start at ' + callData.time + '. Join us <a href=' + callData.zoomLink + '>here</a></span>') | ||
if ((startTime - now <= 2 && startTime - now >= 0) && (dayOfWeek === 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dayOfWeek === 1
should be tied to the day
property of the data in communityCall.json
right? Otherwise changes to the day for a call will still cause this banner to be displayed on Monday.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The call for the for seeable future will on Monday, but I really should be using the day
property of communityCall.json
which is better than just hard coding Monday.
js/lib/alert-bar.js
Outdated
var dayOfWeek = new Date().getDay() | ||
var reminder = $('.alert-bar') | ||
var announcement = $('.lh-title').append('<span>The IPFS community call will start at ' + callData.time + '. Join us <a href=' + callData.zoomLink + '>here</a></span>') | ||
if ((startTime - now <= 2 && startTime - now >= 0) && (dayOfWeek === 1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Banner will disappear as soon as the call starts. Should we leave it up for the duration of the call? - maybe encode this information in the communityCall.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @alanshaw! Eric here (Proto-noob).
What if the message changed to, "The IPFS community call is now in progress! Join us here." once the call began, so latecomers could join?
Also, i would make the entire "Join us here." phrase a link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the message changed to, "The IPFS community call is now in progress! Join us here." once the call began, so latecomers could join?
I like that suggestion. That being said, the link to landing page will still be the same so that participants will know that they're joining a live call.
js/lib/communityCall.json
Outdated
"callName": "IPFS Weekly Call", | ||
"time": "17:00 UTC", | ||
"day": "Monday", | ||
"zoomLink": "https://protocol.zoom.us/j/443621844" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the banner is going to be displayed 2 hours before the call is it better to link them to a page with more info about the call, which has the notes link as well as the zoom link? Otherwise people might be sat in an empty room for 2 hours.
Or link directly to the zoom when the call is happening, but to a "more information" page before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the banner is going to be displayed 2 hours before the call is it better to link them to a page with more info about the call, which has the notes link as well as the zoom link?Otherwise people might be sat in an empty room for 2 hours.
Yes, it would be better to link people to a "more information" page. Will swap out link for "more information page" ipfs/team-mgmt#549
In general I am +1 for this change. Minor UI tweaks to do. Threre are a bunch of related changes so I'd be happy to talk it through when you have a moment @pkafei
|
Will do. I'm gonna sort out CI right now so we can get a preview URL on this PR, then I'll review. |
js/lib/communityCall.json
Outdated
@@ -0,0 +1,7 @@ | |||
{ | |||
"callName": "IPFS Weekly Call", | |||
"time": "18:00 UTC", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weekly call is at 17:00 UTC.
js/lib/alert-bar.js
Outdated
var startTime = parseInt(callData.time, 10) | ||
var dayOfWeek = new Date().getUTCDay() | ||
var reminder = $('.alert-bar') | ||
if ((startTime - now <= 2 && startTime - now >= 0.5) && (dayOfWeek === callData.day)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need some tests here, this will still hide the banner while the call is in progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally the banner still appears while the call is in session on my machine- but will add test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tweaks for the call info page
js/lib/test.js
Outdated
var callData = require('./communityCall.json') | ||
var shouldShowBanner = require('./alert-bar').shouldShowBanner | ||
|
||
var dayOfWeek = callData.day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dayOfWeek
, startTime
, two_hours_before_call
and dayOfWeek
are no longer used in these tests. Please remove.
content/ipfs-call/index.md
Outdated
|
||
## Welcome to the IPFS Weekly Call | ||
|
||
Join us every week for the IPFS Weekly Call where we hear from the IPFS Community. We use zoom to host our calls and [this link](https://protocol.zoom.us/j/443621844) will directly bring you into the live video chat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call info page should probably say when the call is, in case you find your way to it at some time that the call isnt on.
content/ipfs-call/index.md
Outdated
Before you join there are several points we would like to go over: | ||
|
||
1. The IPFS calls are recorded and are put on [Youtube](https://www.youtube.com/playlist?list=PLuhRWgmPaHtSGRSHdU9dbsukHKlihZZAe) | ||
2. Please sign into the [IPFS Weekly Call sheet](https://docs.google.com/document/d/1WHyIZhBo2eEgYXlZ5HLHg6a6ZWTH3tV848sWkYBJjJA/edit#heading=h.hz5t61tdc5r6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. Please sign into the [IPFS Weekly Call sheet](https://docs.google.com/document/d/1WHyIZhBo2eEgYXlZ5HLHg6a6ZWTH3tV848sWkYBJjJA/edit#heading=h.hz5t61tdc5r6) | |
2. Have a read of the agenda for the call, and add your name as an attendee on the [IPFS Weekly Call document](https://docs.google.com/document/d/1WHyIZhBo2eEgYXlZ5HLHg6a6ZWTH3tV848sWkYBJjJA/edit#heading=h.hz5t61tdc5r6) |
|
||
1. The IPFS calls are recorded and are put on [Youtube](https://www.youtube.com/playlist?list=PLuhRWgmPaHtSGRSHdU9dbsukHKlihZZAe) | ||
2. Please sign into the [IPFS Weekly Call sheet](https://docs.google.com/document/d/1WHyIZhBo2eEgYXlZ5HLHg6a6ZWTH3tV848sWkYBJjJA/edit#heading=h.hz5t61tdc5r6) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We expect all participants to abide by the Code of Conduct.
js/lib/alert-bar.js
Outdated
var startTime = parseInt(callData.time, 10) | ||
var dayOfWeek = new Date().getUTCDay() | ||
var reminder = $('.alert-bar') | ||
if ((startTime - now <= 2 && startTime - now >= parseFloat(-0.5) && (dayOfWeek === callData.day))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkafei the callTime
function should use shouldShowBanner
to determine if it should show or hide the banner. right now it duplicates the logic, which is risky, as the test will suggest that the logic is correct but we're not testing the function that will actually be run when the user loads the page.
if ((startTime - now <= 2 && startTime - now >= parseFloat(-0.5) && (dayOfWeek === callData.day))) { | |
if (shouldShowBanner(new Date(), callData.time, callData.day)) { |
That also means that callTime
no longer needs to declare the now
, startTime
and dayOfWeek
variables.
js/lib/alert-bar.js
Outdated
var startTime = parseInt(startTimeString, 10) | ||
var dayOfWeek = date.getUTCDay() | ||
|
||
console.log(now, startTime, dayOfWeek) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good form to remove all debug logging from code before we ship it. Please delete this line.
@olizilla -- this is a stale issue but still sounds like a good idea. What do you think -- and might you have time to implement the changes you'd originally requested, in order to ship? |
Closing this issue, since IPFS website has changed in scope/purpose since issue opened. Let's keep this concept in mind as we discuss redeveloping IPFS website as a whole. |
Announcement bar appears 2 hours before the weekly IPFS Weekly Call begins.
Notes:
Currently I'm toggling the alert bar using
hide
in jquery. In hindsight I probably should have usedappend
orprepend
in order to implement the toggling feature.In the next version of the toggle bar, I would like the alert bar to turn red 15 minutes before the IPFS Weekly Call begins.
Interested in announcing other calls in the alert bar. That being said, not including the IPFS Cluster standups, we have about 9 public calls per week. I'm afraid our audience might suffer alert bar overload if we include an alert bar for every public call.
@daviddias @olizilla