-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
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.
Great job! I love to see this sample! It's fun 😄
In addition to the comments, I'd really like to keep the language consistent, if possible and unless it's an egregious error. Admittedly, it's minor, but we want to maintain consistency as a general policy so that folks aren't taking progressive liberties with their own spin on things.
These changes have been made.
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.
Took an initial pass at the code this time. Really well done! 🎉
Let me know if you have any questions about the comments left.
Co-authored-by: Alissa Renz <alissa.renz@gmail.com>
Co-authored-by: Alissa Renz <alissa.renz@gmail.com>
Co-authored-by: Alissa Renz <alissa.renz@gmail.com>
Co-authored-by: Alissa Renz <alissa.renz@gmail.com>
Co-authored-by: Alissa Renz <alissa.renz@gmail.com>
6116391
to
a853f66
Compare
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 is amazing, awesome work and I love the app concept ✨ Left a few bits of feedback and some questions - will keep an eye out on this PR for updates! let me know if you have any questions 🙌
}, | ||
}); | ||
|
||
export default SlackFunction(FormatLeaderboardFunction, ({ inputs }) => { |
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.
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.
Not sure I fully understand the case; did you log this run as happening within the past 7 days or between 8-14 days ago?
The runs are currently split between the past week (up to 7 days ago) and the prior week (8-14 days ago) when calculating the top line. I'll update the language to make it more clear that this total is for the past and prior weeks. If these days are being grouped wrong, please let me know though! I'll also give the tests another check too!
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.
Ah yes, I think maybe there was some confusion with the wording! I think the date I had submitted was Jan 25 - in my mind, since that was in the last grouping of last Monday - this Sunday, it made sense as "last week". This calculation makes sense in the context of it being the last 7 days! 🙌 Thanks for the clarification!
Co-authored-by: Ashley <12901850+hello-ashleyintech@users.noreply.github.com>
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.
Looks to be in a good state to merge! Merging this and then will take public 🚀 Thanks so much for all of the hard work on this everyone!! 🙌
Type of change (place an x in the [ ] that applies)
Summary
A sample app example of how to use a datastore and automate the collection, storage, retrieval, and computation of data. The app includes a workflow that asks people to log the date and distance of their last run and a datastore to store everyone's runs. A second workflow prints a leaderboard to a channel, as well as weekly stats (e.g. "Your team ran X miles this week! [X% difference from last week]"). A scheduled trigger posts those stats to a channel on a weekly basis.
Requirements (place an x in each [ ] that applies)