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

Footer: Add Compiler line #2460

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Footer: Add Compiler line #2460

merged 3 commits into from
Oct 17, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Oct 15, 2024

closes #2449

image image image

@machikoyasuda machikoyasuda self-assigned this Oct 15, 2024
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates i18n Copy: Language files or Django i18n framework deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Oct 15, 2024
Copy link

github-actions bot commented Oct 15, 2024

Coverage report

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

@@ -111,6 +111,9 @@
<li>
<a class="footer-link m-0 p-0" href="https://cdt.ca.gov/privacy-policy/" target="_blank" rel="noopener noreferrer">{% translate "Privacy Policy" %}</a>
</li>
<li class="ms-auto">
Copy link
Member Author

@machikoyasuda machikoyasuda Oct 15, 2024

Choose a reason for hiding this comment

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

ms-auto means "margin start: auto", and "start" means left if the page is set to a language that reads from left to right.

How this works: https://stackoverflow.com/questions/33924655/position-last-flex-item-at-the-end-of-container

@machikoyasuda machikoyasuda force-pushed the feat/2449-powered-by branch 2 times, most recently from fd61000 to 738acac Compare October 16, 2024 21:53
@machikoyasuda machikoyasuda marked this pull request as ready for review October 16, 2024 21:55
@machikoyasuda machikoyasuda requested a review from a team as a code owner October 16, 2024 21:55
@@ -111,6 +111,9 @@
<li>
<a class="footer-link m-0 p-0" href="https://cdt.ca.gov/privacy-policy/" target="_blank" rel="noopener noreferrer">{% translate "Privacy Policy" %}</a>
</li>
<li class="ms-auto">
<a class="footer-link m-0 p-0" href="https://compiler.la" target="_blank" rel="noopener noreferrer">{% translate "Powered by Compiler LLC" %}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't notice this in the design. I think the link text should not include the "Powered by" copy, that should come before/outside the link text.

This was what I wrote in the original ticket:

Use the following language:

Powered by Compiler LLC

Copy link

Choose a reason for hiding this comment

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

Updated to this:
Screenshot 2024-10-17 at 6 06 21 PM

I think it's a cleaner look to have the entire phrase be a link, and I don't see that being an issue with screenreaders either. HOWEVER, either way is fine by me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm.. this is actually tricky because the footer links change color after they've been visited, but the "Powered by" would stay light blue. So the Powered by always stay light blue, and the Compiler LLC part changes to purple after it's been clicked:

image

Are we okay with this?

Copy link
Member

Choose a reason for hiding this comment

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

I would expect the "Powered by" to be... white text? Making it the same color as the link feels strange.

Copy link
Member

Choose a reason for hiding this comment

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

If this is too complicated, leaving the entire text as the link is fine.

Copy link
Member Author

@machikoyasuda machikoyasuda Oct 17, 2024

Choose a reason for hiding this comment

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

This is how it would look with white/purple visited link or white/blue unvisited link:

image image image image

Hover:
image
Focus:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the commit for the white text + regular link change: 7d20a5b

@thekaveman
Copy link
Member

Thanks for the last-minute updates.

@machikoyasuda machikoyasuda merged commit f7aea38 into main Oct 17, 2024
10 checks passed
@machikoyasuda machikoyasuda deleted the feat/2449-powered-by branch October 17, 2024 22:34
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 i18n Copy: Language files or Django i18n framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Powered by" link to application footer
3 participants