-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Generate badge examples from data + code #1163
Conversation
This pull request sets us up to generate the badge examples dynamically from data and code. For this pass, I’ve put all the data into a single `.js` file, though in the future it could be split up into separate files by service. I used JS instead of JSON because it’s more user-friendly. Right now, try.html is still checked in, mostly for the benefit of reading this diff, though it should be removed on the next pass to avoid unnecessary complexity at merge time.
I deployed a review copy of this: https://shields-staging-pr-1163.herokuapp.com/try.html |
See a7a1d5f which compares a normalized copy of the old HTML with the new HTML. |
Hmm, I realized in the example in the heroku pr already the same... |
Since there are no Github credentials on the review app, most of the Github requests will fail. |
This makes a giant leap toward #776. |
Thanks for working on this! I'll take a look soon.
With my refactoring work, I'd like to move each provider to its own class,
and include the examples directly in the provider class. I guess we could
build that on top of this as we'd just need to change how it's loading the
example data, right?
Sent from my phone.
…On Oct 13, 2017 9:05 AM, "Paul Melnikow" ***@***.***> wrote:
This makes a giant leap toward #776
<#776>.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1163 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFnHaPSNpGIIGLFZHpuzuX9ok-huMRzks5sro1BgaJpZM4P3XOy>
.
|
Definitely! When the providers are collected, they could be run through some code that populates a data structure like this. That code could merge the "old style" examples with the "new style" examples, since we'll want to migrate those providers gradually. |
lib/all-badge-examples.js
Outdated
{ | ||
title: 'Travis', | ||
previewBadgeUri: '/travis/rust-lang/rust.svg', | ||
exampleBadgeUri: 'https://img.shields.io/travis/USER/REPO.svg' |
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.
Maybe omit the hostname from here? We can prepend it in the rendering code (which would also avoid showing prod URLs in dev environments)
I wonder if it'd be better to have a URL pattern to remove the repetition... Something like:
uriFormat: '/travis/{USER}/{REPO}.svg`,
previewParams: {
USER: 'rust-lang',
REPO: 'rust',
}
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.
Maybe omit the hostname from here?
This makes sense. Though we'll need to sort out more clearly what's happening with sed
in the Makefile. It depends on the hostname being there.
uriFormat: '/travis/{USER}/{REPO}.svg`,
previewParams: {
USER: 'rust-lang',
REPO: 'rust',
}
That looks amazing! The Sinatra format could be nice too (i.e. /travis/:user/:repo.svg
. It's going to take some care. Let's do that in the next pass.
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.
I started a checklist in the top comment to make sure I don't miss anything.
lib/all-badge-examples.js
Outdated
{ | ||
title: 'CircleCI', | ||
previewBadgeUri: '/circleci/project/github/RedSparr0w/node-csgo-parser.svg', | ||
exampleBadgeUri: 'https://img.shields.io/circleci/project/github/RedSparr0w/node-csgo-parser.svg' |
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.
For a lot of these, previewBadgeUri
and exampleBadgeUri
are identical. Could we infer one of them if the other one is set?
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.
Yea, that's a great idea. I'm happy to go through and make that change before this is merged.
I'm going to stall for the moment because try.html
keeps changing in master so there is a lot of moving data. I'm hoping my next pass through will be the last. 😄
lib/all-badge-examples.js
Outdated
previewBadgeUri: '/travis/rust-lang/rust.svg', | ||
exampleBadgeUri: 'https://img.shields.io/travis/USER/REPO.svg' | ||
}, | ||
{ |
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.
Maybe we should add "category" too (ie. the heading it's currently under on try.html)? We could likely extract that from the current try.html too.
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.
Yea, for sure, it's in there now. They're called "sections" in this version.
When we break these out by service we can invert that, labeling each one with its category.
@Daniel15 how are you feeling about this? I've got a checklist in the top comment but wanna make sure I only need to regenerate this stuff once! |
Your checklist looks great, @paulmelnikow! :) |
# Conflicts: # package.json # try.html
- Followup from #1163 - Retire try.html - Separate build config for dev and production - Move config for badge examples into the JS build - Move the prod transform into npm scripts - In the future this could be handled using a bundler plugin - make website builds production build as before - Run the production build in CI to make sure it’s working - Build the frontend on Heroku
Looks like this was added way back in #1163 and is definitely not the way things are happening now. I don't see anywhere else it's being used.
Looks like this was added way back in #1163 and is definitely not the way things are happening now. I don't see anywhere else it's being used.
This pull request sets us up to generate the badge examples dynamically from data and code.
For this pass, I’ve put all the data into a single
.js
file, though in the future it could be split up into separate files by service. I used JS instead of JSON because it’s more user-friendly.Right now, try.html is still checked in, mostly for the benefit of reading this diff, though it should be removed on the next pass to avoid unnecessary complexity at merge time.
There probably are a number of good HTML templating languages, including dot which the project is already using. I went with React though, for a few reasons. It sets us up well to build a more dynamic and interesting front-end experience – i.e. by actually using React on the front end. We can make it easy to support internal navigation, badge syntax display, fields to set different values of badges, and perhaps a more simple home page view. With React, we can also re-implement the current functionality in a more modern way. In addition to all this, it’s something two of the maintainers know really well – @Daniel15 and me. (Correct me if I’m wrong!) Not to mention it’s popular, widely supported, and easy to learn. There’s a short term disadvantage of a more complex dependency toolchain (i.e. babel, eslint-config-standard-jsx), though even that is worthwhile groundwork for a more modern frontend.
I wrote a script to extract the structured data from
try.html
. That's checked in atfrontend/extract-badge-examples.js
. It makes a JSON file which I reformatted by hand to createlib/all-badge-examples.js
. I can automate that process if necessary while we're iterating on the PR.During testing I ran
badge-examples-old.html
throughpretty
so I could ensure the output was the same. Basically the difference comes down to quoting and slight differences in the way nbsp is handled. I'll post a couple of those files so people can see for themselves.The documentation needs to be updated, but that’s blocked / conflicting with #1129 which reorganizes all of the documentation.
To do before merging:
frontend/fragments/badge-examples-for-diff.html
exampleBadgeUri
to the rendering codeexampleBadgeUri
frompreviewBadgeUri
where possible, rather than duplicate itfrontend/
Post merge
try.html