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

Refactor(reCAPTCHA): remove icons #2564

Merged
merged 5 commits into from
Dec 5, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Dec 4, 2024

Closes #2538

This PR removes the bankcard and idcardcheck icons and removes them from the Eligibility Start and Enrollment Index pages.

Screenshots of this branch

Page Icons removed (Desktop) Icons removed (Mobile)
Eligibility start image Screenshot 2024-12-05 at 12-47-28 Older Adult benefit overview Cal-ITP Benefits
Enrollment index Screenshot from 2024-12-05 12-19-06 Screenshot 2024-12-05 at 12-50-53 Eligibility confirmation Cal-ITP Benefits
Enrollment index (with alert component) Screenshot from 2024-12-05 12-35-50 Screenshot 2024-12-05 at 12-45-57 Eligibility confirmation Cal-ITP Benefits

@angela-tran angela-tran self-assigned this Dec 4, 2024
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

@angela-tran
Copy link
Member Author

@thekaveman On #2537, it's noted that we should deploy all the changes out together:

All these tickets should be completed before a deploy.

What should merging these PRs look like?

Merge into main and hold off any deployment until all tickets are done? Start a new branch to merge all these PRs into, which is merged into main at the end? I'm leaning towards the second option.

// cc @machikoyasuda

@thekaveman
Copy link
Member

@angela-tran Hmmm, I was thinking of the first option, but the second probably makes more sense just in case we need to release something before this is all ready.

@angela-tran angela-tran changed the base branch from main to refactor/recaptcha-copy December 5, 2024 17:37
i.e. remove icon block and "media-line" div from base template.

also remove now unused CSS style.
remove unneeded template whose sole purpose was to define the icon
@angela-tran angela-tran force-pushed the refactor/remove-icons branch from e7735c0 to a79563f Compare December 5, 2024 17:42
@machikoyasuda
Copy link
Member

The second option sounds better to me too.

@angela-tran angela-tran marked this pull request as ready for review December 5, 2024 19:08
@angela-tran angela-tran requested a review from a team as a code owner December 5, 2024 19:08
@angela-tran
Copy link
Member Author

angela-tran commented Dec 5, 2024

@thekaveman @machikoyasuda Got it, thanks! I created refactor/recaptcha-copy for our reCAPTCHA PRs to go into.

edit: I created a branch protection rule for it too, copying settings for the one for main

@thekaveman
Copy link
Member

nitpick: this is more than just copy right?

@angela-tran
Copy link
Member Author

nitpick: this is more than just copy right?

It is. My thought was all these changes are stemming from us deciding to use our own reCAPTCHA copy instead of the badge, but I'm open to using a different branch name. Do you have one in mind?

height: var(--media-item-icon-size);
margin-right: var(--media-item-icon-margin);
}

.media-list-icon-left-margin {
Copy link
Member

@machikoyasuda machikoyasuda Dec 5, 2024

Choose a reason for hiding this comment

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

I'm assuming this class media-list-icon-left-margin will be deleted in this ticket, #2569, and then we can delete all (or most) of these:

:root {
  --media-item-icon-size: calc(64rem / 16);
  --media-item-gap: calc(24rem / 16);
  --media-item-icon-margin: calc(24rem / 16);
}
@media (min-width: 992px) {
  :root {
    --media-item-icon-size: calc(90rem / 16);
    --media-item-gap: calc(24rem / 16);
    --media-item-icon-margin: calc(32rem / 16);
  }
}

@angela-tran angela-tran merged commit ea9f790 into refactor/recaptcha-copy Dec 5, 2024
8 checks passed
@angela-tran angela-tran deleted the refactor/remove-icons branch December 5, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reCAPTCHA: Remove icons from media item templates
3 participants