Determine active page by considering parent too #19
Conversation
There was a problem hiding this comment.
Looks good!
If you'd want to add a test, the meta tags specs are probably a good example. If that doesn't work give me a shout and I'll add something in.
lib/govuk_tech_docs/helpers.rb
Outdated
|
|
||
| private | ||
|
|
||
| attr_reader :config, :current_page |
There was a problem hiding this comment.
I think these aren't necessary, since you're using @current_page page above.
lib/govuk_tech_docs.rb
Outdated
| @@ -70,6 +71,10 @@ def current_page_review | |||
| def format_date(date) | |||
| date.strftime('%-e %B %Y') | |||
| end | |||
|
|
|||
| def active_page | |||
There was a problem hiding this comment.
Have you considered making moving the from_path? logic into in here?
quis
force-pushed
the
work-out-active-page-from-parent
branch
from
7c82eba
to
503ffe4
Compare
|
OK, I’ve managed to make the code much simpler. Not sure how to test it though – I don’t think I can copy the example you gave now that the implementation isn’t wrapped up in a class. |
There was a problem hiding this comment.
@quis cool!
You could for example test this by adding a "step" to the integration spec
then_there_is_a_heading
then_there_is_a_source_footer
+ and_the_current_page_is_highlightedAnd define the step as something like:
def and_the_current_page_is_highlighted
expect(page).to have_link 'Documentation', class: 'active'
endI've added some links to the tests in #21 in CONTRIBUTING.md.
lib/govuk_tech_docs.rb
Outdated
| @@ -70,6 +70,10 @@ def current_page_review | |||
| def format_date(date) | |||
| date.strftime('%-e %B %Y') | |||
| end | |||
|
|
|||
| def active_page(page_path) | |||
| ("/" + current_page.path) == page_path || current_page.data.parent == page_path | |||
There was a problem hiding this comment.
I think this might not work for the homepage, because page_path is / and current_page.path is index.html.
Currently the logic that works out which page to highlight as ‘active’ in the top navigation is quite naïve (only compares URLs directly). This commit makes it a bit smarter so that a page can define its `parent`, then the navigation will highlight the matching ‘parent’ navigation item. Adapted from: alphagov/govuk-developer-docs@7e5ac51
quis
force-pushed
the
work-out-active-page-from-parent
branch
from
503ffe4
to
a2f5a12
Compare
|
@tijmenb I have added:
|
Currently the logic that works out which page to highlight as ‘active’ in the top navigation is quite naïve (only compares URLs directly).
This commit makes it a bit smarter so that a page can define its
parent, then the navigation will highlight the matching ‘parent’ navigation item.Adapted from: alphagov/govuk-developer-docs@7e5ac51