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

Move detailed guides to new layout #706

Merged
merged 4 commits into from
Jan 29, 2018

Conversation

steventux
Copy link
Contributor

@steventux steventux commented Jan 17, 2018

@tijmenb tijmenb temporarily deployed to government-frontend-pr-706 January 17, 2018 17:56 Inactive
@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from ad1e77c to d8ae1c0 Compare January 17, 2018 18:02
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 17, 2018 18:02 Inactive
@fofr
Copy link
Contributor

fofr commented Jan 18, 2018

Nice to see the removal of that "Too much detail" box.

guides.map { |g| { text: g["title"], path: g["base_path"] } }
end


Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace/linting.

<%= render 'govuk_component/title', @content_item.title_and_context %>
<%= render 'govuk_component/title',
title: @content_item.title,
average_title_length: "long" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

No context is being passed here, so the title is missing the grey "Guidance" text above the title.

items: @content_item.metadata[:other] %>

<%= render "components/contents-list-with-body", contents: @content_item.contents do %>
<%# if @content_item.related_mainstream_content.any? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Commented block still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should also delete the .related-mainstream-content styles

else
assert page.has_text?(value[:text])
assert page.has_text?(value[:text]), "Metadata value '#{value[:text]}' not found"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice ⭐️

@fofr
Copy link
Contributor

fofr commented Jan 18, 2018

What happened to the grey borders around the metadata?

screen shot 2018-01-18 at 09 05 41
screen shot 2018-01-18 at 09 07 08

@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from d8ae1c0 to 87c6bd6 Compare January 18, 2018 13:57
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 18, 2018 13:57 Inactive
@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from 87c6bd6 to 3fb0d2d Compare January 18, 2018 14:14
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 18, 2018 14:14 Inactive
@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from 3fb0d2d to 4a892b1 Compare January 18, 2018 14:37
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 18, 2018 14:37 Inactive
@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from 4a892b1 to 6df3afc Compare January 18, 2018 14:53
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 18, 2018 14:53 Inactive
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 18, 2018 17:02 Inactive
@steventux
Copy link
Contributor Author

steventux commented Jan 19, 2018

@fofr - thanks for the review - I think I have everything covered now, publisher metadata border added in d5ef44b

@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from 9ff4409 to d5ef44b Compare January 19, 2018 11:00
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-706 January 19, 2018 11:00 Inactive
Copy link
Contributor

@maxgds maxgds left a comment

Choose a reason for hiding this comment

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

Looks good

@steventux steventux force-pushed the move-detailed-guides-to-new-layout branch from 3e0dc55 to 97c5a5a Compare January 29, 2018 10:44
@steventux steventux merged commit 0886f79 into master Jan 29, 2018
@steventux steventux deleted the move-detailed-guides-to-new-layout branch January 29, 2018 11:10
jon-kirwan added a commit that referenced this pull request May 15, 2023
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.

5 participants