Take part document #4218
Take part document #4218
Conversation
KludgeKML
force-pushed
the
take-part-document
branch
from
6e82d3c to
50b3e7b
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
50b3e7b to
eccf200
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
eccf200 to
b39bc88
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
d765c5d to
615613e
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
474bc41 to
f388c5c
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
f388c5c to
3e8798f
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
3e8798f to
c8466a7
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
88fb1fe to
be0642f
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
04da7e5 to
7e70eea
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
7e70eea to
1fcb485
Compare
There was a problem hiding this comment.
On the whole the code looks good. 🎉
However, the request tests are missing and should be added.
Also, I think every commit message should be checked to make sure that it's linking to the permalink to the file in government-frontend rather than the commit containing the latest change, as some of those contain a lot of files.
In some cases the file is being linked to, and even better, the actual lines being pulled across, but in those cases it would be better if it linked to the "code" rather than the "blame".
Once these things are fixed this PR will be good to go.
There was a problem hiding this comment.
The content of this commit looks good, but can you update commit message to point to the actual files in government-frontend rather than the commits.
spec/models/content_item_spec.rb
Outdated
| @@ -1,6 +1,56 @@ | |||
| RSpec.describe ContentItem do | |||
| let(:subject) { build(:content_item_with_data_attachments, schema_name: "fancy_page_type") } | |||
|
|
|||
| describe "#initialize" do | |||
There was a problem hiding this comment.
It's not a blocker, but I do find it odd that we're having to test initialize like this because of how much logic it contains, rather than an ordered_related_items attribute on the model or the method.
There was a problem hiding this comment.
I've called it describe "ordered_related_items attribute"
There was a problem hiding this comment.
Can you link to the files in government-frontend, rather than the commits or the "blame".
| def native_language_name_for(locale) | ||
| I18n.t("language_names.#{locale}", locale:) | ||
| end |
There was a problem hiding this comment.
This method is in the content item presenter. Could that file be linked to in the commit message as well.
| @@ -12,3 +12,70 @@ ar: | |||
| show_all_updates: إظهار كل التحديثات | |||
| i18n: | |||
| direction: rtl | |||
| language_names: | |||
| ar: العربيَّة | |||
There was a problem hiding this comment.
It's great that these locale updates are in a separate commit.
Could this commit also link to the locale files rather than a commit diff. Linking to the top-level folder is probably enough. e.g. https://github.com/alphagov/government-frontend/tree/c4bea9e232e929995c4fb5539951cadae8c740c8/config/locales
spec/support/meta_tags.rb
Outdated
| RSpec.shared_examples "it has meta tags for images" do |schema, base_path| | ||
| before do | ||
| example_doc = GovukSchemas::RandomExample.for_schema(frontend_schema: schema) do |random| | ||
| random["details"] = random["details"].merge( |
There was a problem hiding this comment.
Minor: I think you can do random["details"].merge!(...) to change the original object rather than an assign.
KludgeKML
force-pushed
the
take-part-document
branch
from
80a87b5 to
43894ff
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
43894ff to
afa97fd
Compare
| @import "govuk_publishing_components/individual_component_support"; | ||
|
|
||
| .app-c-figure { | ||
| @include govuk-clearfix; |
There was a problem hiding this comment.
I'm not sure if it makes a difference, however I think the @include should be below the property definitions. The same goes for @include govuk-font(16); on line 44.
I'm basing that on the changes in PR alphagov/government-frontend#3323:
This involves moving all the @ declarations to after the other statements
KludgeKML
force-pushed
the
take-part-document
branch
from
afa97fd to
abfd64e
Compare
- In practise, it's almost always request.path, and the specific content items that need it to be something different are already overriding it, so better to configure it as default here
- Used in Take Part, among others - Includes spec file, documentation Commit audit trail: - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/assets/stylesheets/components/_figure.scss - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/views/components/_figure.html.erb - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/views/components/docs/figure.yml - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/test/components/figure_test.rb
- Add code in ContentItem links into the desired internal format for the sidebar. Take part is the first document that uses ordered_related_links. - Add tests Commit audit trail: - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/controllers/content_items_controller.rb#L151-L157
- Add methods to LocaleHelper to provide language names and translate content info into component format - Add methods to ContentItem to provide available translations Commit audit trail: - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/helpers/application_helper.rb - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/views/shared/_translations.html.erb - https://github.com/alphagov/government-frontend/blob/a938e4bee78b794e4e5303a1bde1dfd05e94b287/app/presenters/content_item_presenter.rb#L152-L154
KludgeKML
force-pushed
the
take-part-document
branch
from
abfd64e to
01d56da
Compare
KludgeKML
force-pushed
the
take-part-document
branch
from
01d56da to
74df80a
Compare
- We also update one test that is a little too specific about what the wrapper class should be.
- Needed for the tests added in the next commit. - Separate from the no-image tags test already here, as the shared examples for images break if run on a non-image-bearing document type like help pages Commit audit trail: - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/test/integration/meta_tags_test.rb
- add view and helpers - convert test suite to RSpec on move - note that we explicitly remove the content-bottom-margin div class that was immediately inside the 2/3rds column. This is a style used in a handful of government-frontend document types which (on review with frontend developers) does not really fix the problem it's trying to fix (insufficient bottom margin on some tablet-sized devices). This has been captured as alphagov/govuk_publishing_components#4220 to be fixed properly in the components gem. - We replace the call to TitleHelper in the view with a direct call, as it didn't add much and it's not clear whether it's worth copying that helper across yet. - main element wrapper added as it's in the layout in government- frontend, but not in frontend. Commit audit trail: - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/app/views/content_items/take_part.html.erb - https://github.com/alphagov/government-frontend/blob/6523741691ddfb9551967311a3f79775e9403ecf/test/integration/take_part_test.rb
- copied from government-frontend, originally this was in content_item.schema_name.take_part, here we put it in formats.take_part Commit audit trail: - https://github.com/alphagov/government-frontend/tree/6523741691ddfb9551967311a3f79775e9403ecf/config/locales
KludgeKML
force-pushed
the
take-part-document
branch
from
74df80a to
cb0c966
Compare
What
Handle Take Part documents in Frontend, in preparation for them being taken out of Government Frontend
Why
https://trello.com/c/0LYU5bSd/318-move-route-document-type-takepart-from-governmentfrontend-to-frontend
How
These documents all have a fixed prefix, so add a scoped route for them, a controller, a model and a view. Here we add the initial ContentItem model the document type models will be based on, with the minimum functionality required to get Take Part documents working.
We also port across the figure component, which is used in this document type.
We also create a couple of helpers for rendering generic content.
Screenshots?
From example page: https://www.gov.uk/government/get-involved/take-part/improve-your-social-housing
Before (running in Government Frontend)
After (running in Frontend)