-
Notifications
You must be signed in to change notification settings - Fork 308
Conversation
Does that give you enough to get started, @JessaWitzel? Anything jump out at you that needs to be better-specified? |
Maybe constrain to only list one per owner among the 10? |
If this is all about the projects and being able to support a project and the team working on that project what benefit is there to limiting it to one project per owner? Especially if it's random. Are we worried how it will look if 5/10 of them are owned by one person? |
Yeah, that was my only thought. I'm not proposing we limit the whole website to one project per owner, just the rotating featured 10 on the homepage. Eh? |
Can it reset weekly rather than daily or on pageview? That might jive better with the weekly focus of Gratipay (and reduce overhead vs on pageview, especially). |
I was originally thinking about resetting daily but I like the idea of resetting weekly. I have no clue how to do that but will explore once I get this tab working. :) |
Resetting either weekly or daily is going to be a fair bit more work, because we'll need to store the featured listing somewhere other than in memory. My suggestion would be that we get it working on this PR with generated-every-request, and work on persisting the list in a separate PR. |
I am still having a hard time making enough fake data to test this properly so would LOVE a set of eyes on this before I move forward and some feedback. The icon isn't right, I'm still trying to figure that out. And I haven't written specific tests yet. @mattbk or @whit537 or anyone would you mind taking a look and giving me feedback? |
Is it only showing the tab when there are enough projects that match certain criteria? |
@mattbk Yes. I had to make it that way because of the way our testing environment works but also I think it's smart because we are open source so if someone wants to copy our tabs situation but has less teams it will still work for them. |
@JessaWitzel I don't think the tab should be hidden when there are fewer than 7 popular + 3 unpopular. IMO it's shown when there's at least one and we show up to 7 popular and 3 unpopular. |
@nobodxbodon Well if there is only one or two teams then they will all just show up on the Approved tab. What is the benefit of a featured tab if it just shows the Approved tab in a random order? |
As we have much more than 2 teams now, I suppose you are thinking of reusing it by some other projects. I'd say suit our needs first, and I don't think it's a big task for other project maintainers to customize it to their needs. Besides, in case we have only 5 or 6 popular, is it better to still show the 'featured' tab? |
Ah, I see what you mean. Like if we have 30 projects but only 5 popular. Ok thanks! I will make that change. This works for us currently but I think it makes sense to make that change in case projects fall. I can do that tonight. Any other feedback? |
As a workaround for fake data, you can just modify I changed it to 5 and ran |
Can you explain? Just that it's a piece of paper rather than a different icon? Looks like it's working for me. 👍 |
For things like #4234, would be more realistic to have a range for `nreceiving_from` rather than a constant value.
To make sure we are on the same page, here are some cases in my mind. Could you check?
|
@nobodxbodon The way I wrote it if the total number of approved projects is less than 10 there is no featured tab because it will just be a randomized version of the approved tab. If, however there are more than 10 projects total then the rest of your logic is true. Does that work? So 2 populars, 10 unpopulars = tab shows 2 populars, 8 unpopulars |
@JessaWitzel sounds good to me. Thanks. |
@mattbk yes! I want it to be a star. :( I totally looked up the icon and used the right SVG code and everything. |
I requested reviews but should probably write tests, yeah? Where would those go? |
www/index.html.spt
Outdated
else: | ||
unpopular.append(project) | ||
|
||
if len(teams) < 10: |
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.
To make the part more testable, maybe better to create a utility method like get_featured_projects(popular, unpopular)
to wrap line 48-56.
Also, please create constant variables for 10 and 7, especially if the same value is used in many places.
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.
And seems here 10 should be compared with number of approved projects?
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.
a utility method like
get_featured_projects(popular, unpopular)
A good place for it might be gratipay/utils/__init__.py
, right below get_team
.
www/index.html.spt
Outdated
elif unpopular_sample_size == 0: | ||
featured_projects = random.sample(popular, min(10, len(popular))) | ||
else: | ||
featured_projects = random.sample(popular, popular_sample_size) + random.sample(unpopular, unpopular_sample_size) |
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.
Below seems to be an alternative of L48-55 block:
popular_sample_size = min(10 if (len(unpopular) == 0) else 7, len(popular))
unpopular_sample_size = min(len(unpopular), 10-popular_sample_size)
featured_projects = random.sample(popular, popular_sample_size) + random.sample(unpopular, unpopular_sample_size)
Please add tests first, if you decide to do some refactoring of this part, to avoid changing original logic.
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.
@mattbk I'm not certain where the test cases should go. Could you advise?
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.
@whit537 indicated they should be in test_homepage.py on Slack
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.
Thank you for the review!
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 go with test_utils.py
since utils
seems the natural place to add the function we're talking about. I've added dd0b8ce to bring the test_utils.py
file up to modern standards of organization (one class per function). Hopefully that gives you enough to go on to add tests for the new function.
Rebased again, was 4fcaa25. |
4fcaa25
to
aa23f35
Compare
How about "Unreviewed" to keep the tabs on one line? cc45990 |
Do we want to include Gratipay in the pool of featured projects? Is that too forward? |
Or does it help communicate that Gratipay is funded on Gratipay? |
I think it should be like any other project, included when it is chosen at random. |
|
I'm ready to merge and deploy when you are, @JessaWitzel @mattbk @TravisCI. |
Probably. You think "Unreviewed" is that bad? |
"Unreviewed" is actually more parallel to "Approved" and "Rejected," isn't it? |
Recommendations were implemented.
/me preps a tweet ...
|
We have a +1 from @JessaWitzel in slack. You good, @mattbk? I'm thinking we can tweak the tab after the fact if need be ... |
Alright, I'm holding off until tomorrow. I was the last to commit, so I shouldn't be the one to merge. @mattbk I believe the merge is yours, unless the tab name is a blocker for you. |
"Approved" and "Rejected" are end-members, "In Review" is transitional. Not a blocker for me. |
FIREWORKS!! |
Following up from slack ...
Right now the homepage lists all approved projects by default, most recently created first. This doesn't give a great first impression, because we end up seeing mostly little projects nobody has ever heard of or is giving to. We don't want to hide such projects per se (we want to be careful of "the rich get richer"), but when people are hitting Gratipay for the first time, they want to see, "Who is on here that I recognize?"
I had an idea to incrementally improve the situation without too much trouble (hopefully), and it seemed to @JessaWitzel like a good next project. So here we are! 💃
Basically I propose that we add a "Featured" tab in addition to the others on the homepage. It would be the default tab, and would show ten projects. Seven of the projects would be drawn randomly from a pool of the most popular projects (maybe
ngivers >= 5
, I think? We're so sad), and three would be taken randomly from amongst the rest. (Gratipay itself should never be a featured project.) Once we have those 10, I think maybe we randomize the order we list them on the Featured tab, for freshness and because otherwise how would we sort them without burying the new/unpopular projects at the bottom?Apart from an icon (blue ribbon?) for Featured to match the other tabs, I think we should be able to do this only changing
www/index.html.spt
(plus tests?), because we already load all of the teams every time someone hits the homepage (that's an optimization problem we can worry about later).Mockup
We Are Sad