Add files for secondary navigation: #4229
Conversation
davidtrussler
force-pushed
the
Create-new-component_secondary-navigation
branch
from
ce2e4e8
to
40e72c6
Compare
davidtrussler
force-pushed
the
Create-new-component_secondary-navigation
branch
from
40e72c6
to
ccc6412
Compare
davidtrussler
force-pushed
the
Create-new-component_secondary-navigation
branch
from
ccc6412
to
37e9dec
Compare
| assert_select ".gem-c-secondary-navigation", count: 1 | ||
| assert_select ".gem-c-secondary-navigation__list-item", count: 3 | ||
| assert_select ".gem-c-secondary-navigation__list-item--current", count: 1 | ||
| assert_select ".gem-c-secondary-navigation__list-item--current", text: "Nav item 1" |
There was a problem hiding this comment.
Possibly worth adding an aria-current check in here as well
There was a problem hiding this comment.
Good spot: have added that.
| add_gem_component_stylesheet("secondary-navigation") | ||
|
|
||
| id ||= nil | ||
| items ||= [] |
There was a problem hiding this comment.
Is it worth using the component wrapper here? I think the shared_helper would be needed on this component as well?
There was a problem hiding this comment.
Hi @AshGDS - not sure what the component wrapper is that you are referring to here. And also what the shared_helper does in this case.
There was a problem hiding this comment.
@davidtrussler The component wrapper is something built by Andy to standardise the way data attributes / classes / aria tags are added to the parent HTML tag of the component. The shared helper AFAIK allows margin bottom to be applied to the component for styling purposes.
An example of both helpers exist in the password input component I added - see lines 9 to 16, as well as line 40:
You would then need to reflect the existence of these helpers in your .yml file - you would add uses_component_wrapper_helper: true to the file, and give an example of your component with margin bottom:
A test of the margin functionality in your spec file would be needed as well.
Component wrapper docs: https://github.com/alphagov/govuk_publishing_components/blob/main/docs/component-wrapper-helper.md
Shared helper docs: https://github.com/alphagov/govuk_publishing_components/blob/main/docs/component_conventions.md#shared-helper
Though not sure these are necessary for all components - may be worth asking Andy.
There was a problem hiding this comment.
Thanks for that, Ash. Have now added the wrapper as suggested.
| <% | ||
| add_gem_component_stylesheet("secondary-navigation") | ||
|
|
||
| id ||= nil |
There was a problem hiding this comment.
This id is not exposed in the .yml example, and is also untested.
There was a problem hiding this comment.
Good point, added an id in the yml and added a test for it.
At the same time we noticed the two tests that were there could really be reduced to one since there was some duplication going on.
| <% end %> | ||
| <% end %> | ||
| <% end %> | ||
| <% end %> |
There was a problem hiding this comment.
Is it worth wrapping the render in an if items statement, so that there isn't a scenario where an empty nav/ul is rendered? (I appreciate this may be a very rare edge case)
There was a problem hiding this comment.
Yes, probably rare but worth it - have now add that.
davidtrussler
force-pushed
the
Create-new-component_secondary-navigation
branch
from
37e9dec
to
ebe7f2a
Compare
Thanks Jon, have done that. |
Add Component wrapper helper
davidtrussler
force-pushed
the
Create-new-component_secondary-navigation
branch
from
cc35494
to
4748e05
Compare
What
Add secondary navigation component.
Files added
Why
This component currently exists only on Whitehall (see here). Since it is now needed on other apps (e.g. Mainstream Publisher) this is to make it more widely available.
Visual Changes
This is a new component - screenshot below.