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

Split front end into one page per category #1762

Closed
wants to merge 17 commits into from

Conversation

chris48s
Copy link
Member

@chris48s chris48s commented Jul 3, 2018

Following on from conversation in #1700 I've split the site up into a number of smaller pages with fewer badges on each page.

There's 2 chunks of work going on here:

  1. Mechanics:

    • Present 'downloads', 'version', etc as pages with badges on them
    • Don't show any badges on the index page, just links to categories
    • Allow all badges to be searched from the index page, but without rendering every badge as soon as we press a key
  2. Data:

    • Doing this also makes putting the examples in useful categories more important (a 'miscellaneous' category with >100 badges in it isn't great when its all one page, but it is even less useful after splitting them up). As such, in order to make this more useful, I've attempted to categorise the badges into a larger number of more useful themes.

Would be great to get some feedback on this.

Hopefully I don't end up resolving too many merge conflicts on all-badge-examples.js 🤞

chris48s added 11 commits July 3, 2018 22:41
- Present 'downloads', 'version', etc as pages
- Don't show any badges on the index page,
  just links to categories.
- Tweak search so we can search all badges
  from the index page, but without rendering
  every badge as soon as we press a key.
Merge the remains of 'miscellaneous' and
'longer miscellaneous' into a single 'Other' catgeory

There is probably a bit more categorisation we can
usefully do here, but I think this is a good start.
@chris48s chris48s added frontend The Docusaurus app serving the docs site performance-improvement Related to performance or throughput of the badge servers labels Jul 3, 2018
@shields-ci
Copy link

shields-ci commented Jul 3, 2018

Warnings
⚠️

This PR modified services/gem/gem.js but not services/gem/gem.tester.js. That's okay so long as it's refactoring existing code.

Messages
📖

✨ Thanks for your contribution to Shields, @chris48s!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

</tr>
);
}
return null;
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the search more efficient because it only loads the badges matching our query (instead of hiding the ones that aren't relevant), but in isolation, this still background loads a load of badges when the user types one key (just because a lot of examples match the query), so elsewhere I've also restricted the search on the home page so that it only starts searching after the user enters 2 or more characters.

For the category-level search I haven't restricted it because we've already loaded all the badges in that category before we start searching.

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-1762 July 8, 2018 06:31 Inactive
pages/index.js Outdated
<div>Search term must have 2 or more characters</div>
);
} else {
return(<BadgeExamples
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth adding a ~500ms delay here?
It may help reduce the amount of badges loaded.
something like:

var searchTimeout = 0;
...
clearTimeout(searchTimeout);
searchTimeout = setTimeout(runSearch, 500);

@RedSparr0w
Copy link
Member

Deployed to heroku,
It's looking good so far, But all the links (builds/downloads etc) bring me to the 404 page.

Do you think it would also be useful to still have a way for people to view all the available badges?
Also when searching for badges, should we only show the relevant categories to take up less space?

chris48s added 2 commits July 8, 2018 17:01
this will reduce the number
of badges loaded while typing
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-1762 July 8, 2018 16:40 Inactive
@chris48s
Copy link
Member Author

chris48s commented Jul 8, 2018

Would it be worth adding a ~500ms delay here?

Also when searching for badges, should we only show the relevant categories to take up less space?

Thanks. These are excellent sugestions - I've added both of them in 5351c4a and 5070f6d.

all the links bring me to the 404 page.

Hrm. This works locally for me. Does this reproduce if you check the code out locally?

I'm not missing routing code because we are using next with the default useFileSystemPublicRoutes: true so adding .js files to /pages should automatically create routes for them. See https://github.com/zeit/next.js/#disabling-file-system-routing

My suspicion is that there may be a server restart or page build step missing from the end of the heroku deploy, but I'm not sure what the staging deploy script looks like.

Do you think it would also be useful to still have a way for people to view all the available badges?

I think rendering all badges on one page provides poor user experience as well as bad performance and it is an approach that just doesn't scale as we add more integrations. At the moment I'm 👎 on retaining a single "render all badges" page on balance, but I'm open to more feedback on the issue (esp from users as well as maintainers).

What do you think is the main advantage of keeping it?

@RedSparr0w
Copy link
Member

all the links bring me to the 404 page.

Hrm. This works locally for me. Does this reproduce if you check the code out locally?

Haven't had the chance to test it out locally yet, and not quite sure how heroku handles it all, but if it's working locally i'm sure we can sort it out on the server end.

What do you think is the main advantage of keeping it?

Not sure really, mainly just to display all available services (not on initial load, separate page).
But i think a list of services we support would probably offer the same thing without the overhead.

@chris48s
Copy link
Member Author

chris48s commented Jul 9, 2018

But i think a list of services we support would probably offer the same thing without the overhead.

I'm not sure how we could produce this with the data structure we've got right now. Currently we do have the ability to attach some number of arbitary 'keywords', but for the moment we don't have a reliable meta-property on the examples data structure that identifies a badge as belonging to a 'service'.

This is actually one of the other things I was thinking about while I was re-organising this:

At the moment we've chosen to organise this into a hierarchy of categories and examples, like this:

  • License
    • GitHub
    • NPM
    • PyPI
  • Version
    • GitHub
    • NPM
    • PyPI

but another equally valid way to think of it would be:

  • GitHub
    • License
    • Version
  • NPM
    • License
    • Version
  • PyPI
    • License
    • Version

One makes sense to some people and the other makes sense to others. It would be useful if we could support both.

I think it would be beneficial to improve discoverability by re-organising the data structure so that its more like a flat structure with 'tags' (so a badge can appear under multiple headings) instead of a hierarchy. Then we could make more use of tags for browsing as well as searching. I think that is probably a larger chunk of work. It also probably needs a bit more thought about design. I'm not sure what the homepage would look like in that scenario once we've got a very large number of tags or headings.

@RedSparr0w
Copy link
Member

Yeah, that makes sense.

For the front end maybe we could move it to a table? (possibly in another PR later)

Group By Category:

Category Service Type Badge
Downloads Github All Releases
Github Pre Releases
Mozilla Total
Packagist Per Week
Packagist Per Month
Version Github Latest
Github Latest Pre Release
Mozilla Latest Release
Packagist Latest
Packagist Latest Pre Release

Group By Service:

Service Category Type Badge
Github Downloads All Releases
Downloads Pre Releases
Version Latest
Version Latest Pre Release
Mozilla Downloads Total
Version Latest Release
Packagist Downloads Per Week
Downloads Per Month
Version Latest
Version Latest Pre Release

Or maybe something more like this:

Group By Category:

Downloads

Service Type Badge
Github All Releases
Github Pre Releases
Mozilla Total
Packagist Per Week
Packagist Per Month

Version

Service Type Badge
Github Latest
Github Latest Pre Release
Mozilla Latest Release
Packagist Latest
Packagist Latest Pre Release

Group By Service:

Github

Category Type Badge
Downloads All Releases
Downloads Pre Releases
Version Latest
Version Latest Pre Release

Mozilla

Category Type Badge
Downloads Total
Version Latest Release

Packagist

Category Type Badge
Downloads Per Week
Downloads Per Month
Version Latest
Version Latest Pre Release

For this PR i guess it's best just to split the front end up, and work on the other parts later.

Copy link
Member

@RedSparr0w RedSparr0w left a comment

Choose a reason for hiding this comment

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

I don't have a computer i can test this locally on at the moment, but assuming the category pages show as expected 👍 from me.
This should definitely help take some load off the servers.

@RedSparr0w
Copy link
Member

RedSparr0w commented Jul 11, 2018

I think we still need a clearTimeout() for the delay to cancel the loading?
It seems to flash some badges during typing longer badge names (Chrome web store) which i assume is loading all the badges matching the first 2 letters along with the rest.
search
Edit:
It seems to be loading whatever is currently in the box, after the initial 500ms delay from the first key typed, rather than extending the delay to after the last key typed.

@paulmelnikow
Copy link
Member

Woohoo, this is awesome! I'll take it for a spin sometime in the next few days.

@chris48s
Copy link
Member Author

Sorted. Added the missing clearTimeout() call in f10e4be

@RedSparr0w
Copy link
Member

side note: I've moved [apm] into its own module in #1785 so if this is merged after that, could you also update the license badge category from misc→license.

@paulmelnikow
Copy link
Member

When I click the sections at https://shields-staging-pr-1762.herokuapp.com/ I navigate to a page like https://shields-staging-pr-1762.herokuapp.com/examples/build which is an error page. Any idea what's causing that?

@chris48s
Copy link
Member Author

Any idea what's causing that?

No :( As noted in #1762 (comment) I don't know why this works locally but not on the staging server. I don't know enough about how the staging deploy is done to understand the difference/try and debug it.

If you check this out locally, does it work for you on your local copy?

@paulmelnikow
Copy link
Member

paulmelnikow commented Jul 20, 2018

Ah, I see…

There's an overview of the problem here: https://github.com/zeit/next.js/wiki/Deploying-a-Next.js-app-into-GitHub-Pages

And a solution here, though it's a bit messy, and also designed for react-router, not next's router: https://github.com/rafrex/react-github-pages

And finally, a good discussion around a feature request: isaacs/github#408.

I'm not sure where that leaves us. I don't want to invest in a solution for Next, since we're wanting to replace it. We could switch to Gatsby or CRA before addressing this, though it would create conflicts with this.

Alternatively we could try to manage this on a single page using this.state. I don't love that approach, but it might allow us to get the job done more independently. What do you think?

Added: Come to think of it, this.state seems like a pretty good way of handling what is basically a change in filter.

@chris48s
Copy link
Member Author

Cheers for the additional info. Before thinking about a solution, I'm confused about why we need to work around a limitation of GH Pages if we're deployed on Heroku (in staging) or a VPS (in prod). What's the additional bit of info I'm missing there?

@paulmelnikow
Copy link
Member

Ah, good question. In prod, the servers are on the VPS. That's the img.shields.io rotation. The frontend however, which is shields.io, is served through github pages. There's a slight wrinkle which is that it also is filtered through cloudflare, which used to be necessary to support SSL.

This is why gh-pages is effectively the deploy branch: when we deploy we push the code to the servers and push a built copy of the frontend to gh-pages.

The servers (s0., s1., and s2.shields-server.com) also host a copy of the index page, which gets served through the node server.js process using a static route. However that's intended just for debugging. That built copy of the page gets there through git: the build is checked in to the deploy branch along with the server secrets, and then that branch is pushed to the servers.

The Heroku and Zeit builds use a different mechanism to accomplish the same thing. The build script runs at deploy time, and creates a static copy of the build in build/. This is then served through the node process.

@chris48s
Copy link
Member Author

OK. Having digested this, some thoughts..

Is there a way I can run the code locally so that it replicates the way it runs in staging - how can I reproduce this locally?

The whole 'override the 404 page so it does a client side redirect' fudge is not a great solution. I'd like to avoid doing that.

I think ideally I'd like a solution which allows us to give someone a link to the page listing all the version badges (or whatever). I've been assuming that this will take the form shields.io/examples/version but something like shields.io/?category=version or shields.io/#category=version would also meet that criteria and that should work (I think.. not tested). I think I'll have a look and see if I can set up a test that uses query strings or hash change routing next.

Considering this slightly more widely, it would be useful if we could support having more than one page on the site. For example, if you think about issue #1787 we might want to solve that by adding a page which shows all the named logos. In general this would be a useful problem to solve.

It looks like this is going to be a problem with any front-end solution that uses a router so if we're looking to swap out next for something else, it is worth thinking about this issue in evaluating alternatives.

@paulmelnikow
Copy link
Member

Is there a way I can run the code locally so that it replicates the way it runs in staging - how can I reproduce this locally?

I'm not sure you can get the exact behavior. You could run a plain HTTP server on the build, like python -m SimpleHTTPServer or serve. That would give you something similar as long as you don't navigate to different pages.

The whole 'override the 404 page so it does a client side redirect' fudge is not a great solution. I'd like to avoid doing that.

Agreed, it's bananas.

If we stay with Github Pages, the hash is the way to go, probably something like shields.io/#/category/version. The part after the hash – the "fragment" – doesn't get sent to the server at all. It's only interpreted by the browser. The URLs are a little less pretty but the page is compatible with any hosting environment.

If we want to keep the / paths with no hash, we could look at alternative hosting for the origin server, such as Netlify or Zeit static deploys. I believe both of those work fine with these kind of routes; no 404 hack needed.

I don't know if there is a way to tell Next to use hash routing. I'd be surprised if there were… it's very opinionated.

@paulmelnikow
Copy link
Member

@chris48s What do you think about cherry-picking the category and badge-example changes into another PR? We could get that merged while we think about how to handle the rest.

@chris48s
Copy link
Member Author

Yeah - that sounds like a good idea. Re-categorising the badges is useful even if we continue with the single-page layout and it is cumbersome to resolve merge conflicts on all-badge-examples.js as we merge other PRs that change it.

I'll have a look at splitting this into two smaller PRs tomorrow or Tuesday.

@paulmelnikow
Copy link
Member

Oh gosh, never say "resolve #1762". I didn't mean to close this. Sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend The Docusaurus app serving the docs site performance-improvement Related to performance or throughput of the badge servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants