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

Show old related links for selected content items #351

Merged
merged 2 commits into from
May 9, 2017

Conversation

carvil
Copy link
Contributor

@carvil carvil commented May 9, 2017

We are in the process of releasing the new navigation for Education
content. One item that needs to be solved from a product perspective is
how to deal with curated related links. Until that's solved, some
content items still need to show the old related links instead of the
new navigation for users in the B group of our A/B test.

These overrides existed in the frontend application. Since answers
were ported to government-frontend, we also need to port the override.

Trello: https://trello.com/c/U6JKZN6s/115-make-sure-we-override-the-related-links-on-some-mainstream-content-that-was-recently-moved-to-government-frontend

@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-351 May 9, 2017 10:32 Inactive
Copy link
Contributor

@fofr fofr left a comment

Choose a reason for hiding this comment

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

The fallback won't work, there's a typo.

<% if MainstreamContentFetcher.with_curated_sidebar.include?(content_item.base_path) %>
<%=
render(
partial: 'govuk_component/relted_items',
Copy link
Contributor

Choose a reason for hiding this comment

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

'govuk_component/relted_items'
Typo here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how your tests were passing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well spotted! I've fixed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no rendering going on locally, only a mock of the component with the data...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is where using the correct component CSS selectors would have surfaced the bug in tests.

Copy link
Contributor

@fofr fofr May 9, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try switching to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fofr thanks for the references, I've added a new commit for this: f21f494

We are in the process of releasing the new navigation for Education
content. One item that needs to be solved from a product perspective is
how to deal with curated related links. Until that's solved, some
content items still need to show the old related links instead of the
new navigation for users in the B group of our A/B test.

These overrides existed in the `frontend` application. Since `answers`
were ported to `government-frontend`, we also need to port the override.
@carvil carvil force-pushed the port-mainstream-sidebar-override branch from 9d15820 to f2ef29d Compare May 9, 2017 10:52
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-351 May 9, 2017 10:52 Inactive
This change makes sure we find the correct sidebars in tests.
@carvil carvil merged commit 825dbb2 into master May 9, 2017
@carvil carvil deleted the port-mainstream-sidebar-override branch May 9, 2017 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants