Update past foreign secretaries #6243
Conversation
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
2 times, most recently
from
385af80 to
3dda0d6
Compare
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
from
3dda0d6 to
7aeb7ba
Compare
|
All looks good to me. Just one small comment to add with relation to - I wondered if an alternative approach could be to replace this CSS with the govuk-!-margin-bottom-0 class? |
Good call, should work the same, i'll update and remove those extra lines. |
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
2 times, most recently
from
16b0bab to
24f871b
Compare
|
@jon-kirwan updated! Tests were failing but needed a rebuild. |
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
from
24f871b to
537ee0b
Compare
There was a problem hiding this comment.
While the output is sound, I have reservations around the div vs li markup and am concerned about the level of repetition that exists. I think the 3 pages dealt with in the one trello ticket could be sharing more code because of their similarities and would like you to explore that a bit more rather than merging as-is.
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
from
fac8615 to
2ae256c
Compare
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
from
2ae256c to
4ab99ec
Compare
There was a problem hiding this comment.
This is looking much better from a DRY perspective, but I think I haven't communicated very well what I was aiming for. The three pages on this one trello card are using the same patterns throughout, there seems to be value in sharing not just the css, but the partial here. I think this is a good first step towards uniting them like that, but if that's the end game the person and image partial names and locations should be more generic. I know the underlying code that builds each page differs but do you think this is achievable?
| .featured-profiles { | ||
| margin-bottom: $gutter; | ||
| // unique image card styles for "past-foreign-secretaries" & "past-past-chancellor" | ||
| .gem-c-image-card__list-item-link { |
There was a problem hiding this comment.
This doesn't feel great, overriding component styles. Willing to let it pass if we absolutely have to but I'd suggest considering either/or 1) Checking with design that the default isn't good enough, 2) making this style the default for the "unlinked links".
There was a problem hiding this comment.
We shouldn't be overriding component styles in applications - it breaks component isolation - e.g. if we were to change the name of this class in the component, this override would suddenly stop working and we wouldn't know about it.
If the link styles definitely need to be different in this one situation, please add it as an option to the component. Do they definitely need to be different?
There was a problem hiding this comment.
This doesn't feel great, overriding component styles. 1) Checking with design that the default isn't good enough, 2) making this style the default for the "unlinked links".
Agreed, I've removed. 🤔
I think I was looking at the page "as a whole" and noticed this mismatch - I'm not sure why I decided to knowingly break convention with a tweak.
If the link styles definitely need to be different in this one situation, please add it as an option to the component. Do they definitely need to be different?
Yeah they don't - not really. It's a slight visual variation but yeah it might be a good case to up-the-font for this across the whole image_card to match govuk-body but not essential
- Group via century - Apply Design System - Use Components - Add lazy loading to images - Separate data and add to partials - Remove redundant CSS - Update a selection of headings link to the particular profile of that person. As headings are not typically link-able (and not within the heading component) this "custom" to this page and maintains previous functionality - Adjusting CSS style naming to be more generic to apply wider. - Due to additional work on this PR[1] lazy loading has been added to the image_cards to improve the performance. - "extra_links" that is text only is also in use on the image_card - Add aria to ensure the markup reads as grouped lists to AT yet maintain the Design System layout and styling without adding additional code. (This also potentially allows for more fine grain control of what is within grouped lists) - No alt on image as decorative --- Explored using ol > li markup however decided against it due to the nested grouping that was needed to apply the Design System's rows and columns. This page is somewhat un-usual, in that this is list of images. For example to convert to a list and use the grid system - markup similar to this is needed which is invalid: ol (21st century) > div (rows wrapper) > li (column) --- [1]alphagov/govuk_publishing_components#2270 Remove alt text from image
chris-gds
force-pushed
the
update-past-foreign-secretaries
branch
from
4ab99ec to
1fdec4e
Compare
@maxgds I know what you mean - I've combined these 3 PRs into one as I think it makes it easier to review addressing this point - https://github.com/alphagov/whitehall/pull/6264/files There might be a case to combine these but the data is different 🤔 let me know your thoughts. |
|
Moving into Draft (aim to close) as combined PR raised: #6264 |
What
Update Past Foreign Secretaries Page to apply new Design + Design system + use Components
Why
On-going work to align architecture, have pages benefit from Accessibility improvements
Visuals
Done when
Grouped related PRs
#6236
#6243
#6241
Anything else?
Add link to image card heading - this is not dependant
alphagov/govuk_publishing_components#2277
Implementing a semantic list in this context + the Design System layout could be done various different ways. This might prompt a change during review.