From 9bc90cd9329be056b5e9bcd0e5f1bb4a0574a752 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen <6842965+vanessayuenn@users.noreply.github.com> Date: Mon, 25 Jan 2021 17:57:32 +0100 Subject: [PATCH] Learning Track navigation banner (#17440) * add middleware to handle `learn` query param * add exception to query-less cache key * add querystring to learning track guides --- data/ui.yml | 3 ++ includes/article.html | 5 ++- includes/learning-track-nav.html | 18 +++++++++ layouts/product-sublanding.html | 10 ++--- lib/get-link-data.js | 45 +++++++++++++++------- lib/page.js | 1 + middleware/index.js | 1 + middleware/learning-track.js | 40 +++++++++++++++++++ middleware/render-page.js | 12 +++++- tests/rendering/learning-tracks.js | 62 ++++++++++++++++++++++++++++++ 10 files changed, 176 insertions(+), 21 deletions(-) create mode 100644 includes/learning-track-nav.html create mode 100644 middleware/learning-track.js create mode 100644 tests/rendering/learning-tracks.js diff --git a/data/ui.yml b/data/ui.yml index 769025e23f27..2b83989f2feb 100644 --- a/data/ui.yml +++ b/data/ui.yml @@ -160,3 +160,6 @@ product_sublanding: tutorial: Tutorial how_to: How-to guide reference: Reference +learning_track_nav: + prevGuide: Previous Guide + nextGuide: Next Guide diff --git a/includes/article.html b/includes/article.html index 52d1d4104af4..bc884154917e 100644 --- a/includes/article.html +++ b/includes/article.html @@ -65,7 +65,10 @@

+
+ {% if currentLearningTrack and currentLearningTrack.trackName %} + {% include learning-track-nav %} + {% endif %} {% include helpfulness %} {% unless page.hidden %}{% include contribution %}{% endunless %}
diff --git a/includes/learning-track-nav.html b/includes/learning-track-nav.html new file mode 100644 index 000000000000..674fafe85e1e --- /dev/null +++ b/includes/learning-track-nav.html @@ -0,0 +1,18 @@ +
+{% assign track = currentLearningTrack %} + + + {% if track.prevGuide %} + {% data ui.learning_track_nav.prevGuide %} + {{track.prevGuide.title}} + {% endif %} + + + + {% if track.nextGuide %} + {% data ui.learning_track_nav.nextGuide %} + {{track.nextGuide.title}} + {% endif %} + + +
\ No newline at end of file diff --git a/layouts/product-sublanding.html b/layouts/product-sublanding.html index d9af6ab059ee..4f8cf90a8a84 100644 --- a/layouts/product-sublanding.html +++ b/layouts/product-sublanding.html @@ -29,7 +29,7 @@

{{ page.shortTitle }}

{% octicon "star-fill" height="24" class="v-align-middle m-2"%}

{{ featuredTrack.title }}

{{ featuredTrack.description }}
- + {% octicon "arrow-right" height="20" %} {% data ui.product_sublanding.start_path %} @@ -37,7 +37,7 @@

{{ featuredTrack.title }}

{% for guide in featuredTrack.guides %}
  • - +
    {{ forloop.index }} @@ -61,7 +61,7 @@

    {% data ui.product_sublanding.learning_paths %}

    {% for track in page.learningTracks offset:1 %} -
    +
    @@ -75,14 +75,14 @@

    {{ track.description }}

    -     + {% data ui.product_sublanding.start %} {% octicon "arrow-right" height="20" %}
    {% for guide in track.guides %} - +
    {{ forloop.index }}
    diff --git a/lib/get-link-data.js b/lib/get-link-data.js index 46a64d773019..8c47ae895a10 100644 --- a/lib/get-link-data.js +++ b/lib/get-link-data.js @@ -5,28 +5,45 @@ const removeFPTFromPath = require('./remove-fpt-from-path') // rawLinks is an array of paths: [ '/foo' ] // we need to convert it to an array of localized objects: [ { href: '/en/foo', title: 'Foo', intro: 'Description here' } ] -module.exports = async (rawLinks, context) => { +module.exports = async (rawLinks, context, option = { title: true, intro: true }) => { if (!rawLinks) return + if (typeof rawLinks === 'string') { + return await processLink(rawLinks, context, option) + } + const links = [] for (const link of rawLinks) { - const linkPath = link.href || link - const version = context.currentVersion === 'homepage' ? nonEnterpriseDefaultVersion : context.currentVersion - const href = removeFPTFromPath(path.join('/', context.currentLanguage, version, linkPath)) + const linkObj = await processLink(link, context, option) + if (!linkObj) { + continue + } else { + links.push(linkObj) + } + } + + return links +} + +const processLink = async (link, context, option) => { + const linkPath = link.href || link + const version = context.currentVersion === 'homepage' ? nonEnterpriseDefaultVersion : context.currentVersion + const href = removeFPTFromPath(path.join('/', context.currentLanguage, version, linkPath)) + + const linkedPage = findPage(href, context.pages, context.redirects) + if (!linkedPage) return null - const linkedPage = findPage(href, context.pages, context.redirects) - if (!linkedPage) continue + const opts = { textOnly: true, encodeEntities: true } - const opts = { textOnly: true, encodeEntities: true } + const result = { href, page: linkedPage } - links.push({ - href, - title: await linkedPage.renderTitle(context, opts), - intro: await linkedPage.renderProp('intro', context, opts), - page: linkedPage - }) + if (option.title) { + result.title = await linkedPage.renderTitle(context, opts) } - return links + if (option.intro) { + result.intro = await linkedPage.renderProp('intro', context, opts) + } + return result } diff --git a/lib/page.js b/lib/page.js index a7072b7bde7d..ec29665bf2c6 100644 --- a/lib/page.js +++ b/lib/page.js @@ -208,6 +208,7 @@ class Page { const track = context.site.data['learning-tracks'][context.currentProduct][trackName] if (!track) continue learningTracks.push({ + trackName, title: await renderContent(track.title, context, { textOnly: true, encodeEntities: true }), description: await renderContent(track.description, context, { textOnly: true, encodeEntities: true }), guides: await getLinkData(track.guides, context) diff --git a/middleware/index.js b/middleware/index.js index 4eead804ec36..68e61948a212 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -84,6 +84,7 @@ module.exports = function (app) { app.use(require('./enterprise-server-releases')) app.use(require('./dev-toc')) app.use(require('./featured-links')) + app.use(require('./learning-track')) // *** Rendering, must go last *** app.get('/*', asyncMiddleware(require('./render-page'))) diff --git a/middleware/learning-track.js b/middleware/learning-track.js new file mode 100644 index 000000000000..758912378d6f --- /dev/null +++ b/middleware/learning-track.js @@ -0,0 +1,40 @@ +const { getPathWithoutLanguage, getPathWithoutVersion } = require('../lib/path-utils') +const getLinkData = require('../lib/get-link-data') + +module.exports = async (req, res, next) => { + const noTrack = () => { + req.context.currentLearningTrack = {} + return next() + } + + if (!req.context.page) return next() + + const trackName = req.query.learn + if (!trackName) return noTrack() + + const track = req.context.site.data['learning-tracks'][req.context.currentProduct][trackName] + if (!track) return noTrack() + + const currentLearningTrack = { trackName } + + const guidePath = getPathWithoutLanguage(getPathWithoutVersion(req.path)) + const guideIndex = track.guides.findIndex((path) => path === guidePath) + + if (guideIndex < 0) return noTrack() + + if (guideIndex > 0) { + const prevGuidePath = track.guides[guideIndex - 1] + const { href, title } = await getLinkData(prevGuidePath, req.context, { title: true, intro: false }) + currentLearningTrack.prevGuide = { href, title } + } + + if (guideIndex < track.guides.length - 1) { + const nextGuidePath = track.guides[guideIndex + 1] + const { href, title } = await getLinkData(nextGuidePath, req.context, { title: true, intro: false }) + currentLearningTrack.nextGuide = { href, title } + } + + req.context.currentLearningTrack = currentLearningTrack + + return next() +} diff --git a/middleware/render-page.js b/middleware/render-page.js index c2ef5e8f1be6..a81b549e2c5a 100644 --- a/middleware/render-page.js +++ b/middleware/render-page.js @@ -18,11 +18,21 @@ const pageCache = new RedisAccessor({ allowSetFailures: true }) +// a list of query params that *do* alter the rendered page, and therefore should be cached separately +const cacheableQueries = ['learn'] + module.exports = async function renderPage (req, res, next) { const page = req.context.page // Remove any query string (?...) and/or fragment identifier (#...) - const originalUrl = new URL(req.originalUrl, 'https://docs.github.com').pathname + const { pathname, searchParams } = new URL(req.originalUrl, 'https://docs.github.com') + + for (const queryKey in req.query) { + if (!cacheableQueries.includes(queryKey)) { + searchParams.delete(queryKey) + } + } + const originalUrl = pathname + ([...searchParams].length > 0 ? `?${searchParams}` : '') // Serve from the cache if possible (skip during tests) const isCacheable = !process.env.CI && process.env.NODE_ENV !== 'test' && req.method === 'GET' diff --git a/tests/rendering/learning-tracks.js b/tests/rendering/learning-tracks.js new file mode 100644 index 000000000000..bc9c9682f78b --- /dev/null +++ b/tests/rendering/learning-tracks.js @@ -0,0 +1,62 @@ +const { getDOM } = require('../helpers/supertest') + +jest.setTimeout(3 * 60 * 1000) + +describe('learning tracks', () => { + test('render first track as feature track', async () => { + const $ = await getDOM('/en/actions/guides') + expect($('.feature-track')).toHaveLength(1) + const href = $('.feature-track li a').first().attr('href') + const found = href.match(/.*\?learn=(.*)/i) + expect(found).not.toBeNull() + const trackName = found[1] + + // check all the links contain track name + $('.feature-track li a').each((i, elem) => { + expect($(elem).attr('href')).toEqual(expect.stringContaining(`?learn=${trackName}`)) + }) + }) + + test('render other tracks', async () => { + const $ = await getDOM('/en/actions/guides') + expect($('.learning-track').length).toBeGreaterThanOrEqual(4) + $('.learning-track').each((i, trackElem) => { + const href = $(trackElem).find('.Box-header a').first().attr('href') + const found = href.match(/.*\?learn=(.*)/i) + expect(found).not.toBeNull() + const trackName = found[1] + + // check all the links contain track name + $(trackElem).find('a.Box-row').each((i, elem) => { + expect($(elem).attr('href')).toEqual(expect.stringContaining(`?learn=${trackName}`)) + }) + }) + }) +}) + +describe('navigation banner', () => { + test('render navigation banner when url includes correct learning track name', async () => { + const $ = await getDOM('/en/actions/guides/setting-up-continuous-integration-using-workflow-templates?learn=continuous_integration') + expect($('.learning-track-nav')).toHaveLength(1) + const $navLinks = $('.learning-track-nav a') + expect($navLinks).toHaveLength(2) + $navLinks.each((i, elem) => { + expect($(elem).attr('href')).toEqual(expect.stringContaining('?learn=continuous_integration')) + }) + }) + + test('does not include banner when url does not include `learn` param', async () => { + const $ = await getDOM('/en/actions/guides/setting-up-continuous-integration-using-workflow-templates') + expect($('.learning-track-nav')).toHaveLength(0) + }) + + test('does not include banner when url has incorrect `learn` param', async () => { + const $ = await getDOM('/en/actions/guides/setting-up-continuous-integration-using-workflow-templates?learn=not_real') + expect($('.learning-track-nav')).toHaveLength(0) + }) + + test('does not include banner when url is not part of the learning track', async () => { + const $ = await getDOM('/en/actions/learn-github-actions/introduction-to-github-actions?learn=continuous_integration') + expect($('.learning-track-nav')).toHaveLength(0) + }) +})