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

Fix components guide #1281

Merged
merged 2 commits into from Feb 12, 2021
Merged

Fix components guide #1281

merged 2 commits into from Feb 12, 2021

Conversation

DilwoarH
Copy link
Contributor

@DilwoarH DilwoarH commented Feb 11, 2021

This change fixes the components guide endpoint in collections publisher. There was a bug in the yaml file which stopped the guide from rendering, this has now been fixed and a small test added to make sure the components guide loads.

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Invalid yaml was breaking the view
@DilwoarH DilwoarH force-pushed the fix-components-guide branch 2 times, most recently from 9dde3ad to b37c4c3 Compare February 11, 2021 15:19
@@ -28,31 +28,35 @@ examples:
id: markdown-editor
with_bullet_list_button:
data:
controls: [:bullets]
controls:
- :bullets
Copy link
Member

Choose a reason for hiding this comment

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

Nice one fixing these 👍

@@ -0,0 +1,13 @@
require "rails_helper"

RSpec.feature "Visit Component Guide" do
Copy link
Member

Choose a reason for hiding this comment

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

Great stuff adding a test for this.

I was a bit surprised to have it as a feature test but since there isn't a concept of requests tests for this project (instead has the older approach of controller tests) then I agree feature is the place to put this stuff.

require "rails_helper"

RSpec.feature "Visit Component Guide" do
include ComponentGuideSteps
Copy link
Member

Choose a reason for hiding this comment

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

It seems superfluous to have a helper file for this. I'd just add any methods into here (as per: https://github.com/alphagov/collections-publisher/blob/master/spec/features/access_control_spec.rb#L42-L62)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

RSpec.feature "Visit Component Guide" do
include ComponentGuideSteps

describe "Component Guide" do
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need a describe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

scenario "User views the component guide page" do
when_i_visit_the_component_guide_index_page
i_see_a_page_title
i_see_subheadings
Copy link
Member

Choose a reason for hiding this comment

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

These two assertion seem to actually test parts of govuk_publishing_components gem by checking for HTML and text which the gem provides.

I'd probably just do:

  when_i_visit_the_component_guide_index_page
  then_i_receive_a_valid_response
end

# ...

def then_i_receive_a_valid_response
  expect(page.status_code).to eq(200)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Nice one 👍

@DilwoarH DilwoarH merged commit aa29cbf into master Feb 12, 2021
@DilwoarH DilwoarH deleted the fix-components-guide branch February 12, 2021 10:38
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.

None yet

2 participants