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

Add single page notification button to consultation template #2350

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

danacotoran
Copy link
Contributor

@danacotoran danacotoran commented Jan 28, 2022

Add support for the single page notification button to the consultation
template. The button will not appear until the feature is enabled on
pages with this content type.

Seeing as the button is always grouped together with the published dates
component I created a new published_dates_with_notification_button
partial to be shared between publication, detailed_guide and
consultation.

https://trello.com/c/iqhyMnMP

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

@govuk-ci govuk-ci temporarily deployed to government-f-add-button-ttyku7 January 28, 2022 17:41 Inactive
@danacotoran danacotoran force-pushed the add-button-to-consultation branch from 77a0809 to b5cb3bd Compare January 31, 2022 12:14
@govuk-ci govuk-ci had a problem deploying to government-f-add-button-ttyku7 January 31, 2022 12:14 Failure
Add support for the single page notification buton to the consultation
template. The button will not appear until the feature is enabled on
pages with this content type.

Seeing as the button is always grouped together with the published dates
component I created a new  `published_dates_with_notification_button`
partial to be shared between publications, detailed_guides and
consultations.
@danacotoran danacotoran force-pushed the add-button-to-consultation branch from b5cb3bd to 85d7a02 Compare January 31, 2022 14:28
@govuk-ci govuk-ci temporarily deployed to government-f-add-button-ttyku7 January 31, 2022 14:28 Inactive
@danacotoran danacotoran marked this pull request as ready for review January 31, 2022 15:10
Copy link
Member

@huwd huwd left a comment

Choose a reason for hiding this comment

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

nice!
Non blocking but a thought, we're using instance variables here in a partial.
Do we / should we prefer to pass local variables here instead?

Pro: as instance variables are set in the controller, they'll fail if we ever try to include this partial into a view that's spun up by a different controller. So using local variables in partials allows us to make these truely re-usable between any view, and not locked to the implimentation in a controller. Perhaps especially as we've put this partial in _shared/ which implies it could be re-used broadly?

Con: It will make your lovely newly slimmed down calls in publication, detailed guide and consultation chunky again. 🤔

I'm not sure what the right thing is here, no what's blocking?
I wonder if there's a preference in the frontend community?

@huwd
Copy link
Member

huwd commented Feb 2, 2022

We caught up and discussed the pros / cons of locales, we decided because of this case sticking with what we've got is all cool.
Nice work.

@danacotoran danacotoran merged commit 0edbd7b into main Feb 2, 2022
@danacotoran danacotoran deleted the add-button-to-consultation branch February 2, 2022 10:21
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