search

jskjott / webring

Create a home online
https://webring.recurse.com
33 stars 28 forks source link

implement hasRingLink #14

Closed mfekadu closed 5 years ago

mfekadu commented 5 years ago

added the property hasRingLink to each site in sites.js

also updated the explore function to only explore the sites that are site.hasRingLink == true

also the same for keepExploring()

also refactored the code so that keepExploring() is a function for better readability

tested manually by exploring from start to finish and it seems to work the same except now sites that are site.hasRingLink == false are omitted and they can be re-included once the site has the link somewhere/somehow

mfekadu commented 5 years ago

@jskjott Does this new functionality seem to resolve the UX issue of the user not knowing where to find the next RC logo link when the link is not there?

jskjott commented 5 years ago

Yeah it is definitely necessary to find a fix to people not having the icon on their sites. I think this is mostly a good fix. The one change I would make, I have commented on this elsewhere as well, is to move hasRingLink out of sites.js and into index.js or perhaps even a seperate file.

In the future we should make it unnecessary to have a hasRingLink. We should require people to have the icon on their site before we accept a pull request.

Does this make sense?

jskjott commented 5 years ago

I messaged Tom and Wesley asking them to add the icon to their sites. Tom added the icon to his site, so now only Wesley's site is lacking an icon/link to the webring.

mfekadu commented 5 years ago

Yes, makes sense to keep sites.js minimal!

Sounds like next steps are

Hope to find time for these sometime soon. Thanks for the feedback!

mfekadu commented 5 years ago

Seems like these conflicts are no longer an issue. Let's close this pull request?