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

Rss feed subscription #1230

Merged
merged 11 commits into from
Aug 12, 2014
Merged

Rss feed subscription #1230

merged 11 commits into from
Aug 12, 2014

Conversation

oiami
Copy link
Contributor

@oiami oiami commented Jun 16, 2014

As Oalders's suggestion on #327 we have had RSS feed and we should make it more visible as well as tweak the author feed. So what I have done in this pull request are

  • The author feed will be like 'author's activity' feed, the activity includes
    1. This author has released a module
    2. This author ++ed a module.
    Example feed from my RSS reader

screenshot 2014-06-17 01 47 28

  • Make RSS feed button be more visible
    • On **/author** and **/recent** page I put the RSS button to the top of the table as well as add page header.
    • On **module** page I put subscription button on the dependencies section.

any comment about the function and design ? Could you check. :)

@rwstauner
Copy link
Contributor

On module page I put subscription button on the dependencies section.

Since everything else in that section has to do with dependencies it seems out of place.

Maybe it could go over on the left side, in the release-tools template?
Then it would still show up on both release and module pages.

Also, I don't think we need to do the s/-/::/g in the link text.
The feed is for the distribution, so displaying the distribution name is appropriate.

@rwstauner
Copy link
Contributor

I like the feed icon you added to the top right of those pages.

The titles by the icon don't read well for me, though.
What do you think about "Recent Uploads"
and maybe "Activity for $name ($pause_id)"?

I like to see the author's name, it makes it more personal and I think it reads better.

@rwstauner
Copy link
Contributor

Two notes about the new build_author_entry sub:

The $dist variable is a transformation of the dist name to a module name,
however both places where it is used we really should still be using the dist name, because currently it is the distribution that is actually marked as a favorite, not any specific module.

So I would change the "$author ++ed $dist" back to "$author ++ed $item->{distribution}"
and the link that goes to 'pod', $dist really should point to 'release', $item->{distribution}.

With that said, the rest of that for loop is difficult for me to follow.
I think modifying the passed-in data structures usually leads to confusion and bugs (I always prefer immutability), but I also find it hard to follow the logic. Three attributes are assigned each time, but each one has to do a test to determine if this entry is a release or a favorite... but the conditions are not obvious and it makes the code hard to follow. I had to stare at it for a while before it all made sense to me.

Since nearly every attribute varies based on the type of entry (release or favorite) do you think it would make sense to use separate methods? I think that would greatly simplify the code and make it a lot easier to read.

What do you think?

@rwstauner
Copy link
Contributor

Sorry it took me so long to review this, but I really appreciate your work.
This is good stuff!
Thanks!

@ranguard
Copy link
Member

These are looking good - but yea it doesn't feel right under the 'Dependencies'

@oalders
Copy link
Member

oalders commented Jun 25, 2014

What @ranguard said. :)

@rwstauner
Copy link
Contributor

I like the change you made for separating the types of entries into separate functions.

One little thing, though: Could you change the names of those functions from _entry to _entries? Since the functions take multiple entries and return multiple entries I think it would be more clear if the names of the functions reflected the plurality of the output.

Thanks!

@rwstauner
Copy link
Contributor

now that bootstrap 3 is merged you can rebase this and make the UI changes you wanted.

@oiami
Copy link
Contributor Author

oiami commented Aug 7, 2014

I've done this for a while because I rebase it on bootstrap branch, new fixes are

  • Change author activity feed massege so they will be like
    • AUTHOR ++ed Some-Distribution when the author ++ a distribution
    • AUTHOR has released His-Distribution-0.001 when the author has new release
  • Change author page title
    subscript_on_author_page
  • recent page title
    recent metacpan org
  • move subscription link from dependency section to toolbar
    subscript_on_toobar

Done!!, please let me know if it needs any further change, thanks :D

rwstauner added a commit that referenced this pull request Aug 12, 2014
@rwstauner rwstauner merged commit b40d2bd into metacpan:master Aug 12, 2014
@rwstauner
Copy link
Contributor

@oiami This looks great. I'm going to merge this so @Talina06 can build on your work to add some more data to the feed. However, on my pc the feed icon on the top right is a little off to the left... when i try to hover, i don't actually get the pointer icon until i'm off to the right of it. Can you take a look at that? As usual I'm on firefox. Thanks!

@oiami
Copy link
Contributor Author

oiami commented Aug 12, 2014

oh, wow!, so weird that it happens only on Firefox, I will figure out :D

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.

4 participants