561 new document sub navigation page #8402
Conversation
…ely ensuring the New document page renders correctly with the respective radio button options, inset text as well as the next / cancel form buttons.
…e index action as well as the redirecting the user to the correct new document path after they have selected a particular radio button. We have also taken measures to ensure that only users with Preview design system permission will be able to access this page.
…new_document_options routes are implemented here.
|
This looks great 👍🏽 |
RodneyJohnsonGDS
force-pushed
the
561-new-document-sub-navigation-page
branch
from
fba0bbb to
5c9bb48
Compare
…t. Here we are handling multiple radio buttons and ensuring the data is collected by the form correctly.
farahTW
force-pushed
the
561-new-document-sub-navigation-page
branch
from
5c9bb48 to
ffcd7fc
Compare
| <div class="govuk-grid-row"> | ||
| <div class="govuk-grid-column-two-thirds"> | ||
| <%= form_with url: admin_new_document_options_path do %> | ||
| <%= render "govuk_publishing_components/components/radio", { |
There was a problem hiding this comment.
Where we have a bunch of radio controls we should also have a <legend> element inside the <fieldset> which is created as part of the Form radio button component. This could be flagged as an accessibility failure/warning. It can be set via the heading parameter in the component and should ideally use the page title as its value, removing the page title itself.
There was a problem hiding this comment.
Accessibility is important - well spotted! Done!
There was a problem hiding this comment.
Hi @RodneyJohnsonGDS - the <legend> change is perfect, ta! Only thing is that we still need the <% content_for :page_title, "New document" %> line that has been removed so that the page title appears in the browser tab.
There was a problem hiding this comment.
Hey @RodneyJohnsonGDS
After refactoring the test controller code wise it looks ok to me.
Just wondering if can squash all commits in to one that will look more neater as all files are related to one component.
| selected_option = params.require(:new_document_options).to_sym | ||
| redirect_to redirect_options[selected_option] | ||
| else | ||
| redirect_to admin_new_document_path, alert: "Please select a new document option" |
There was a problem hiding this comment.
Rather than using the alert here we should be using the error component, which is more consistent with what we have done elsewhere and more closely follows the design.
There was a problem hiding this comment.
Thanks for the feedback David! As discussed we will pick up how we handle errors across the codebase as a separate user story. For the time being, we'll keep implementation as is.
|
@RodneyJohnsonGDS I've left a couple of comments on here about refactoring the page title into a |
I'd agree with squashing the commits. If we think about it as how it would be read in the context of the overall commit history I think a single commit that says this introduces a new page for selecting a new document would be easier to read and understand. |
RodneyJohnsonGDS
force-pushed
the
561-new-document-sub-navigation-page
branch
from
c502138 to
8f9566b
Compare
…ly improving the accessibility features of the index page as well as ensuring we have a test that handles the redirect of all the radio buttons.
RodneyJohnsonGDS
force-pushed
the
561-new-document-sub-navigation-page
branch
from
8f9566b to
bd6ee86
Compare
Trello: 561 | Link New Document drop down in the sub navigation to new page with radio selection
This PR creates a new view which users will be directed to when they click on the "New document" link in the navigation bar.
Changes:
Screenshot below:
This screenshot is what the user sees when they first land on the landing page:
This screenshot is what the user sees when they select Next without selecting a radio button:
Follow these steps if you are doing a Rails upgrade.