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 large dark icon for action link #3596

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Add large dark icon for action link #3596

merged 2 commits into from
Sep 15, 2023

Conversation

1pretz1
Copy link
Contributor

@1pretz1 1pretz1 commented Sep 8, 2023

What

Uses the existing dark icon but makes it slightly larger as per the
design recommendation for the new benefits calculator link being added
to the browse benefit page.

Related PR: alphagov/collections#3373

Why

Current dark icon is too small and doesn't fit the recommended design.

Visual Changes

Large dark icon used for the new action link:

Screenshot 2023-09-13 at 10 39 04 Screenshot 2023-09-13 at 10 39 39

Trello:
https://trello.com/c/mwQET0e0/2153-add-link-to-the-benefits-smart-answer-to-the-browse-benefits-page

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 8, 2023 13:57 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 8, 2023 14:39 Inactive
@1pretz1 1pretz1 force-pushed the add-large-dark-icon branch from b2a7534 to 7e24770 Compare September 13, 2023 09:44
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 13, 2023 09:44 Inactive
1pretz1 added a commit to alphagov/collections that referenced this pull request Sep 13, 2023
We're now linking from the mainstream browse benefits page to the the
financial support calculator. This feels a bit hacky as we're adding
bespoke logic for one page, however the alternative would be to add
functionality to Collections Publisher and then Collections which
doesn't seem worthwhile. If we find multiple browse pages want this
feature then we probably should look into adding the functionality to
the publishing tool.

Related PR: alphagov/govuk_publishing_components#3596

Trello:
https://trello.com/c/mwQET0e0/2153-add-link-to-the-benefits-smart-answer-to-the-browse-benefits-page
@1pretz1 1pretz1 force-pushed the add-large-dark-icon branch from 7e24770 to bd8de2f Compare September 13, 2023 09:55
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 13, 2023 09:56 Inactive
@1pretz1 1pretz1 changed the title Add large dark icon Add large dark icon for action link Sep 13, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 13, 2023 09:59 Inactive
@1pretz1 1pretz1 marked this pull request as ready for review September 13, 2023 10:05
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

All looks fine to me. I've added one comment below with regards to a style that might not be needed but have asked for a second opinion.

@1pretz1 1pretz1 force-pushed the add-large-dark-icon branch from 909cb48 to 3487943 Compare September 15, 2023 10:32
Uses the existing dark icon but makes it slightly larger as per the
design recommendation for the new benefits calculator link being added
to the browse benefit page.

Related PR: alphagov/collections#3373
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 15, 2023 10:32 Inactive
@1pretz1 1pretz1 force-pushed the add-large-dark-icon branch from 3487943 to aed489a Compare September 15, 2023 10:33
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3596 September 15, 2023 10:33 Inactive
@1pretz1 1pretz1 requested a review from jon-kirwan September 15, 2023 10:36
@1pretz1
Copy link
Contributor Author

1pretz1 commented Sep 15, 2023

Addressed the max-width comment @jon-kirwan @MartinJJones (thanks for pointing it out and investigating)- could this be re-reviewed please? Thank you!

Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@1pretz1 1pretz1 merged commit 99b62e3 into main Sep 15, 2023
@1pretz1 1pretz1 deleted the add-large-dark-icon branch September 15, 2023 10:49
1pretz1 added a commit to alphagov/collections that referenced this pull request Sep 19, 2023
We're now linking from the mainstream browse benefits page to the the
financial support calculator. This feels a bit hacky as we're adding
bespoke logic for one page, however the alternative would be to add
functionality to Collections Publisher and then Collections which
doesn't seem worthwhile. If we find multiple browse pages want this
feature then we probably should look into adding the functionality to
the publishing tool.

Related PR: alphagov/govuk_publishing_components#3596

Trello:
https://trello.com/c/mwQET0e0/2153-add-link-to-the-benefits-smart-answer-to-the-browse-benefits-page
1pretz1 added a commit to alphagov/collections that referenced this pull request Sep 19, 2023
We're now linking from the mainstream browse benefits page to the the
financial support calculator. This feels a bit hacky as we're adding
bespoke logic for one page, however the alternative would be to add
functionality to Collections Publisher and then Collections which
doesn't seem worthwhile. If we find multiple browse pages want this
feature then we probably should look into adding the functionality to
the publishing tool.

Related PR: alphagov/govuk_publishing_components#3596

Trello:
https://trello.com/c/mwQET0e0/2153-add-link-to-the-benefits-smart-answer-to-the-browse-benefits-page
1pretz1 added a commit to alphagov/collections that referenced this pull request Sep 22, 2023
We're now linking from the mainstream browse benefits page to the the
financial support calculator. This feels a bit hacky as we're adding
bespoke logic for one page, however the alternative would be to add
functionality to Collections Publisher and then Collections which
doesn't seem worthwhile. If we find multiple browse pages want this
feature then we probably should look into adding the functionality to
the publishing tool.

Related PR: alphagov/govuk_publishing_components#3596

Trello:
https://trello.com/c/mwQET0e0/2153-add-link-to-the-benefits-smart-answer-to-the-browse-benefits-page
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.

4 participants