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

Get started fixes #52

Merged
merged 4 commits into from May 6, 2021
Merged

Get started fixes #52

merged 4 commits into from May 6, 2021

Conversation

zemccartney
Copy link
Member

@zemccartney zemccartney commented Apr 19, 2021

Addresses #50

Relative to the list on that issue:

  1. Updated at one point in the tutorial, appears most links were in place

Additionally,

  • removed explanation of npx, given that tool is more commonplace (it was brand new at the time of original writing)
  • added boilerplate's git project init command to top of the tutorial, to head off issues with applying flavors failing due to overwriting local changes
  • updated route config method capitalization to match convention in example repo
  • removed a handful of usages of the word "just"; realized it was brutally overused (to be clear, by me, my bad writing habit; to my ear at least 😛 )

Lastly, to the issue's suggestion of opening links in new tabs, this was a bit tricky: setting target=_blank within the markdown file didn't work; the attribute was stripped by Github's markdown API, which we use to process the file to HTML.

To workaround this, I experimented with 2 strategies, neither of which struck me as great, but both work:

  • pre-process: as seen in lib/routes/getting-started.js, prior to passing the html received from Github off to handlebars, we process it by adding target="_blank" to all a tags
  • handlebars: we lean into our usage of Handlebars by
    • use handlebars expressions to represent links e.g. {{link "here" "github.com/hapipal/examples/tree/master/paldo-riddles" true}}
      • the protocol is separated because otherwise, github translates the valid URL to an anchor element with an href attribute, on which the link helper chokes
    • handle these expressions with the link helper
    • compile these expressions within the html processed by Github via the precomp helper, which compiles the html before passing it off to our existing template to set as a normal expression; without this step, the expressions appear literally. Concretely, in getting-started.hbs, replace {{{html}}} with {{{precomp html true }}}

@devinivy some questions for ya:

  • re: opening links in new tabs, are you cool with either strategy above? Or should I cut that work completely, cool to leave links as-is?
  • Should I update all hapi documentation link in the tutorial (and elsewhere on the site) to hapi.dev, not API.md on github? Not crucial I know, but thinking to be consistent with how each repo directs users to hapi.dev
  • I was thinking adding a seed, to create list of riddles, instead of depending on people to manually recreate their own could smooth out testing the later steps in the tutorial; how does that sound to you? I realized on reading back through that having to manually recreate riddles on server startup could annoy, if not trip people up. Another non-critical, thinking just as a nice-to-have

lib/services/github.js Outdated Show resolved Hide resolved
Comment on lines +42 to +43
# make your first commit to init project history
git add --all
Copy link
Member

@devinivy devinivy May 1, 2021

Choose a reason for hiding this comment

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

I believe all the files are already added after starting a new project. However, after npm install there is one new file, package-lock.json, that this would add.

Copy link
Member

@devinivy devinivy May 1, 2021

Choose a reason for hiding this comment

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

I see now that this mirrors the boilerplate readme. Perhaps it is more straightforward to leave it as you have it than get granular about adding specific files 👍

@@ -232,7 +222,7 @@ module.exports = {

Be sure to restart your server in order to pick-up this new code.

If you cURL our new route (`curl http://localhost:3000/riddle-random`) or visit it in your browser, you'll see one of Paldo's riddles. We're up and running!
If you cURL our new route (`curl http://localhost:3000/riddle-random`) or visit it [in your browser](http://localhost:3000/riddle-random), you'll see one of Paldo's riddles. We're up and running!
Copy link
Member

Choose a reason for hiding this comment

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

Above there were some instructions removed to set the PORT back to 3000. Does that mean we should use 4000 throughout the rest of the tutorial?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't for the life of me recall what I was thinking there. I'll add some note about the rest of the tutorial serving on 3000 after that env setting section

Copy link
Member Author

Choose a reason for hiding this comment

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

H'ok, added back a note explaining this away!

@@ -342,7 +332,7 @@ We can take advantage of this in a couple of ways.

### Manually

Just run `npm run lint`—this executes eslint with our config on all files not ignored by npm (see the project's standard `.npmignore`)—and then fix whatever warnings and errors it spits out.
Run `npm run lint`—this executes eslint with our config on all files not ignored by npm (see the project's standard `.npmignore`)—and then fix whatever warnings and errors it spits out (or if you prefer eslint's automatic fixing: `npm run lint -- --fix`).
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, cool tip!


```sh
git cherry-pick objection
npm install
Copy link
Member

Choose a reason for hiding this comment

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

Is there an instruction elsewhere to install the new packages (objection, knex, etc.)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ay, yea, it's a bunch of lines down, after explaining what all's happening with this command: https://github.com/hapipal/hapipal.com/pull/52/files#diff-663e1a280a7fe30ead22732740aa3d215335bf0c66967a35595d2bf153ae2108L435

Does that read alright to you? I was kept that as the only place where we instruct to install since the instructions right after this allude to resolving merge conflicts in package.json. But I could see lifting those up, so it reads something like:

Expect to resolve small merge conflicts when pulling flavors in, typically just in `package.json`  and `server/manifest.js` having to do with overlapping dependencies in HEAD and the flavor.

+ Now let's check everything's still working:
+ 
+ ```sh
+ # bring in our new dependencies
+ npm install
+ npm start
+ ```

Do you prefer one over the other?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like it's spoken to, all good to leave it as-is 👍

@devinivy
Copy link
Member

devinivy commented May 1, 2021

Ay! Thanks for this, looks great and it will feel so good to resolve these.

re: opening links in new tabs, are you cool with either strategy above? Or should I cut that work completely, cool to leave links as-is?

I think we'll be stuck with a hack no matter what, since as you mentioned there's no support for this in GitHub markdown. My preference would actually be some post-processing on the frontend since I think it will be the most straightforward to understand and maintain. Something to this effect:

const markdownLinksExternal = () => {

    const links = document.querySelectorAll('.markdown-body a');

    for (let i = 0; i < links.length; ++i) {
        const link = links[i];
        link.setAttribute('target', '_blank');
    }
};

Should I update all hapi documentation link in the tutorial (and elsewhere on the site) to hapi.dev, not API.md on github? Not crucial I know, but thinking to be consistent with how each repo directs users to hapi.dev

If you are open to taking the time to do that, that would be awesome!

I was thinking adding a seed, to create list of riddles, instead of depending on people to manually recreate their own could smooth out testing the later steps in the tutorial; how does that sound to you? I realized on reading back through that having to manually recreate riddles on server startup could annoy, if not trip people up. Another non-critical, thinking just as a nice-to-have

I'm down with that, but if it would be simpler for the purposes of the tutorial to use file-backed SQLite persistence then that's cool too.

We can also pull any of these out into separate issues and address later if you'd like 👍

Co-authored-by: devin ivy <devin@bigroomstudios.com>
…n new tabs (clean up server-side strategy), add section for persisted riddles
@zemccartney
Copy link
Member Author

Alright, I think I addressed everything (thanks for all those detailed notes, btw! 🙏 )

  • added link target post-processing, cleaned up prior junk
  • switched all links to hapi and joi docs to point at their respective sites instead of github
  • wrote up instructions for persisting data to a sqlite file (and pointed at the premade one in examples)
  • tidied up the other suggestions

Though let me know if there's anything else that'd be good to address while I'm in here!

const markdownLinksExternal = () => {

// external links only (no section anchors or internal (#) links)
const links = document.querySelectorAll('.markdown-body.entry-content a:not([href^="#"]):not(.anchor)');
Copy link
Member

Choose a reason for hiding this comment

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

😏

Copy link
Member Author

Choose a reason for hiding this comment

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

🎩
🍹

Copy link
Member

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Beautiful, thanks again 🙏

@devinivy devinivy self-assigned this May 6, 2021
@devinivy devinivy merged commit c4f9a9c into hapipal:master May 6, 2021
@zemccartney zemccartney deleted the get-started-fixes branch May 6, 2021 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants