Related items component #704
Conversation
|
|
||
| h2 { | ||
| @include core-24; | ||
| font-weight: 600; |
There was a problem hiding this comment.
Is there a reason for using 600 here? 700 maps to bold and there's a bold mixin for 24px text which would allow these two lines to be combined.
@include bold-24 from the front end toolkit's _typography.scss.
There was a problem hiding this comment.
Good spot – we should update this. It has been ported from: https://github.com/alphagov/static/blob/master/app/assets/stylesheets/helpers/_core.scss#L64
|
The ruby bits look good to me. 👍 |
|
@gemmaleigh @tombye I've simplified the styles in 7d9fa7e I haven't added the print mixin as it appears to not yet be imported for components – weirdly the print mixin doesn't come from the toolkit as I expected. |
Component print styles work differently, see #638 (this is something we may want to re-examine, I can't remember the reason for going with this vs print mixin right now) |
|
@dsingleton @tombye Updated print styles in edd2e45 |
|
Looks good to me then 👍 |
| items: | ||
| - title: "Wikivorce" | ||
| url: "http://www.wikivorce.com" | ||
| rel: external |
There was a problem hiding this comment.
Did you have other rel values in mind?
If not, it might be simpler to have a boolean is_external field instead.
There was a problem hiding this comment.
I preferred the flexibility of setting rel.
There was a problem hiding this comment.
To expand, rel values that might be relevant: nofollow, prefetch.
There was a problem hiding this comment.
👍 those make sense, possibly rel="up" too.
I was thinking that in a mustache environment, if we were to do something conditional on a specific value of rel then we'd have issues, but thats theoretical, and it wouldn't be a hard API chance to accommodate that.
|
The component API looks good, markup/styling looks good. Couple of small comments. As discussed offline with @fofr having a list of objects, rather than an object of key/value (as we do for the metadata component) is a lot more robust. Arrays preserve order, unlike objects, and support additional values (eg, We've already run into problems with the later on the metadata component, so I think thats a pattern we should avoid for future components (and consider moving away from with metadata). |
bundle exec rails generate govuk_component related_items
* Based styles on 188e526 and core stylesheets https://github.com/alphagov/static/blob/master/app/assets/stylesheets/helpers/_core.scss#L56-L147
Put back visually hidden part of more link, based on related.raw.html.erb This distinguishes each more link for accessibility.
When passing in a section id, use this to add an id attribute and a labelledby attribute. This is in favour of generating an id, eg. by parameterising the title so that the component isn't using rails specific features. Care must be taken when using the component so that navigation is always correctly labelled. Pattern based on http://tink.uk/screen-readers-aria-roles-html5-support/
* Use `bold-24` to avoid an extra (and incorrect) font-weight property * Remove text-colour, the heading colour isn’t being overridden by anything * Remove overflow hidden, nothing within the list is being floated * Remove padding and margin overrides, instead specifying just the property that’s needed
Follow existing components convention for print styling.
fofr
force-pushed
the
related-items-component
branch
from
edd2e45 to
73005ef
Compare
All section titles must be explicitly provided.
Create a related-items component based on188e526 and on core stylesheets
Care must be taken when using the component so that navigation is always
correctly labelled (pattern based on http://tink.uk/screen-readers-aria-roles-html5-support/). When passing in a section id, use this to add an id attribute and a labelledby attribute. This is in favour of generating an id (eg. by parameterising the title) so that the component isn't using rails specific
features.
Trello
paired with @fofr
cc @dsingleton