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 new light action link variant #3602

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

patrickpatrickpatrick
Copy link
Contributor

@patrickpatrickpatrick patrickpatrickpatrick commented Sep 11, 2023

What

Add new variant of action link for the homepage. Add new svg app/assets/images/govuk_publishing_components/action-link-arrow--light.svg. Add new png app/assets/images/govuk_publishing_components/action-link-arrow--light.png.

Why

As part of the changes being made to the homepage.

Screenshot

Screenshot 2023-09-12 at 11 29 41

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 11, 2023 14:01 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 12, 2023 10:29 Inactive
@patrickpatrickpatrick patrickpatrickpatrick marked this pull request as ready for review September 12, 2023 10:30
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 12, 2023 10:30 Inactive
@maxgds
Copy link
Contributor

maxgds commented Sep 12, 2023

No criticism on you, Patrick, but I don't understand why another variant is needed? We have multiple options already (and I'm unsure how many are actually used). The classname is treating "homepage" as a state but being on a homepage isn't really a state as such. What is the differentiating factor that leads to this variation being needed? It seems purely decorative - are we at a point where we should just be refactoring the component and passing arrow colours and background colours in instead of making a new one for each use case - which is somewhat against the principles of components being reusable.

@patrickpatrickpatrick
Copy link
Contributor Author

patrickpatrickpatrick commented Sep 14, 2023

No criticism on you, Patrick, but I don't understand why another variant is needed? We have multiple options already (and I'm unsure how many are actually used). The classname is treating "homepage" as a state but being on a homepage isn't really a state as such. What is the differentiating factor that leads to this variation being needed? It seems purely decorative - are we at a point where we should just be refactoring the component and passing arrow colours and background colours in instead of making a new one for each use case - which is somewhat against the principles of components being reusable.

The 'homepage' naming isn't the best, you are right. A better name that more in-line with the convention would be 'dark arrow light background'. Alternatively this does not need to be a change to the action link component, this component just reside in the frontend application as there isn't an immediate application for this change on other pages.

The work here is following a precedent that has been established but it isn't necessarily a good precedent. That is a good point. The action link component is an icon (which isn't necessarily an arrow, there are examples where this isn't the case) and a link. I suppose if we were to refactor it correctly, then perhaps it should take a path to an icon as argument and the icon should reside in the application that is rendering the action_link?

Copy link
Contributor

@maxgds maxgds left a comment

Choose a reason for hiding this comment

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

I think in the interests of serving the instant need this is ok to go in, but please raise an issue to the effect of the previous conversation - this component is in severe need of rationalising, probably something to do in conjunction with design.

@patrickpatrickpatrick patrickpatrickpatrick changed the title Add new homepage action link variant Add new light action link variant Sep 26, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 26, 2023 11:20 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 26, 2023 11:23 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 26, 2023 11:25 Inactive
New SVG and PNG for action link variant for the homepage. New variant
needed for the new homepage design.
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3602 September 26, 2023 15:54 Inactive
@patrickpatrickpatrick patrickpatrickpatrick merged commit 21c3cc6 into main Sep 26, 2023
@patrickpatrickpatrick patrickpatrickpatrick deleted the action-link-variant-homepage branch September 26, 2023 16:11
@MartinJJones MartinJJones mentioned this pull request Sep 29, 2023
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