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

[website] Add GitHub contributor activity page #24

Merged
merged 2 commits into from Nov 20, 2018
Merged

[website] Add GitHub contributor activity page #24

merged 2 commits into from Nov 20, 2018

Conversation

dwang
Copy link

@dwang dwang commented Nov 14, 2018

A new page is added to display the events that each contributor triggers (stars, pull requests, comments, etc).
The page is populated using data from the GitHub API and jQuery to parse the JSON.

Preview can be seen at: openwisp-website.surge.sh/activity.html

Alternatively, here is an image:
image

@dwang dwang changed the title WIP: [website] Add GitHub contributor activity page [website] Add GitHub contributor activity page Nov 14, 2018
@atb00ker
Copy link
Member

If XYZ comments on a PR, is it possible to directly jump to that comment or issue/pull request ?
Personally, i find that very useful, saves the time to find the comment.
what do you think?

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work! 👍
I like it. I really want to improve it so we can use it, there's some work that needs to be done before we get there.

See my comments and questions below.

  • move the JS logic to a separate file
  • try to have a single function that can be called to create the feed
  • PullRequestReviewCommentEvent is missing but those kind of events are important
  • it's important to show convert the markdown formatting otherwise comments are not readable, use a markdown tool, maybe this one can work: https://github.com/showdownjs/showdown
  • we should convert URLs to links with regular expression, we can do this as a last step, try to google existing solutions to this problem (it's a common problem)

about.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated
<script src="js/scripts.js"></script>
<script>
function createEvent(avatar, name, event, repo, header, content, date) {
if (!event)
Copy link
Member

Choose a reason for hiding this comment

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

in which case event is falsy?

Copy link
Author

Choose a reason for hiding this comment

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

Event can be false because I am passing it to the function as a parameter, and it's the string representation of that event. For example, event could be "created a new issue in". It's there because unhandled events would be empty strings and I didn't want to create a new message in the feed for it.

activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
@dwang
Copy link
Author

dwang commented Nov 15, 2018

Thanks for reviewing, will work on it now.

@dwang
Copy link
Author

dwang commented Nov 16, 2018

@atb00ker Just added it, the title in the message box is a link to the corresponding comment or PR.

@nemesisdesign

  • move the JS logic to a separate file
    Moved the logic to js/scripts.js
  • try to have a single function that can be called to create the feed
    Didn't really know exactly what you wanted, but I moved the createEvent() function into the main function. I also simplified the timeSince() function.
  • PullRequestReviewCommentEvent is missing but those kind of events are important
    PullRequestReviewCommentEvent, PushEvent, and their relevant links/content are added. Let me know if any other events should be accounted for.
  • it's important to show convert the markdown formatting otherwise comments are not readable, use a markdown tool, maybe this one can work: https://github.com/showdownjs/showdown
    Added showdown and it looks like it's working correctly. I haven't tested it much, but Markdown text looks alright. I'm unsure about images though and how they would be formatted. The js/showdown.min.js file is causing Travis to fail, should I do anything about it? Maybe use a CDN instead, or ignore it in JSHint?
  • we should convert URLs to links with regular expression, we can do this as a last step, try to google existing solutions to this problem (it's a common problem)
    Will start working on this now, just wanted to commit my other changes first, and get feedback when I'm working on this.

@ppabcd
Copy link

ppabcd commented Nov 16, 2018

Is the github script used on all pages? I think it's better to separate it from js/scripts.js because not all pages need it and it takes a lot of time because it has to load the github data first.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

definitely do not add it to scripts.js but to a new file please

@dwang
Copy link
Author

dwang commented Nov 16, 2018

Moved the logic over to js/feed.js now. Still have a few more things I want to do. The URL parsing, a collapse button for the messages, and Markdown image formatting. Anything else I should do?

@dwang
Copy link
Author

dwang commented Nov 16, 2018

Demo is also updated with the current changes: http://openwisp-website.surge.sh/activity.html

@dwang
Copy link
Author

dwang commented Nov 16, 2018

Simple URLs are currently being matched based on the protocol (http:// and https://) and they get converted to clickable links. If needed, I can change it to match based on TLD or other aspects of the URL.

@dwang
Copy link
Author

dwang commented Nov 16, 2018

Found an issue, links break if the URL is split into multiple lines. Don't really know how to fix this?
Example:
screen shot 2018-11-16 at 11 15 24 am

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@dwang ok let's not worry too much about so many details now, you did great work. Please resolve the outstanding issues, I will create another GCI task for improving this feature.

js/scripts.js Outdated Show resolved Hide resolved
js/scripts.js Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
activity.html Outdated Show resolved Hide resolved
@dwang
Copy link
Author

dwang commented Nov 18, 2018

@nemesisdesign My editor has been updated and the issues have been resolved. Anything else I should take care of? Thanks!

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Please fix the errors in the travis build regarding jshint.

I had to make some changes, please check the changes I made and let me know if you have any questions, keep in mind to:

  • avoid duplicating code from scripts.js
  • don't use javascript ES6 syntax unless a transpiler is used (here we don't use it)

@nemesifier
Copy link
Member

PS: you can squash the commits when ready

@dwang
Copy link
Author

dwang commented Nov 19, 2018

Got it, working on it right now.

A new page is added to display the events that
each contributor triggers (stars, pull requests,
comments, etc). The page is populated using data
from the GitHub API and jQuery to parse the JSON.
The feed uses Semantic UI to visualize the data.
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work Daniel 👍

Some more work is needed to refine it before we can publish it, but I will create a new issue for that, the first step is done 😊

🎉

@nemesifier nemesifier merged commit 46af09a into openwisp:master Nov 20, 2018
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.

None yet

4 participants