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

Fixup CTD landing & update promo link #14965

Merged
merged 7 commits into from
Nov 18, 2024

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Aug 13, 2024

One-line summary

Fixes link from /de home leading to CTD landing. (Also fixes random markup issues.)

Significant changes and points to review

The CTD was created in several phases and lived in a few places before settling where it is today, accumulating some fragments of its history that break things today. This aims to make it all work as expected:

Landing changed to fx base and cleaned up to just extend from base. Fixed:

  • missing title/suffix whitespace,
  • meta whitespace/newlines breaking some social previews,
  • removed social image definition to enable the locale includes provide that.

Also fixed:

  • Missing v6 heading in /fr shimmed with control (if you have the correct string feel free to suggest…)
  • Some unclosed tags fixed by moving things around, some extra closing tags removed.
  • Repetitive id on presentational elements fixed by adding size grouping in the id.
  • Mobile markup rendering as text being sanitised marked as safe.
  • Style imports were reordered for all the components to have the necessary variables defined, and anything unused removed.
  • Added cursor: pointer to details summary.

Issue / Bugzilla link

#14957
#14950

Testing

http://localhost:8000/de/
http://localhost:8000/de/firefox/challenge-the-default/
http://localhost:8000/fr/firefox/challenge-the-default/?v=6
http://localhost:8000/pl/firefox/challenge-the-default/

@janbrasna janbrasna marked this pull request as ready for review August 21, 2024 07:02
Comment on lines 36 to 38
<p><small>(Wir haben auch ein paar wirklich gute Argumente.)</small></p>

<a href="{{ url('firefox') }}" class="mzp-c-button mzp-t-product" data-cta-text="Ich schau mir Firefox mal an" data-cta-type="button" data-cta-position="banner">Ich schau mir Firefox mal an</a>
<a href="/de/firefox/challenge-the-default/" class="mzp-c-button mzp-t-product" data-cta-text="Ich schau mir Firefox mal an" data-cta-type="button" data-cta-position="banner">Ich schau mir Firefox mal an</a>
Copy link
Contributor Author

@janbrasna janbrasna Aug 21, 2024

Choose a reason for hiding this comment

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

This is a /de/–only promo include with hardcoded de strings, so I felt it's okay to also hardcode a /de/* link, given it's not available in the defined url()s otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW the only thing I have left as-is is the switch logic that still implies the original /firefox takeover:

which results in missing strings if the campaign is turned off, and the landing not removed and/or redirected away (see e.g. https://www.allizom.org/de/firefox/challenge-the-default/ …)

I contemplated monkey-patching a meta-redirect instead of said include, but the page should be removed or redirected from when the time comes instead of flipping the switch anyways now when it's not /firefox takeover, so I kept it as-is — maybe the switch logic can be removed completely to be on the safe side?

@stephaniehobson stephaniehobson added Needs Review Awaiting code review Frontend HTML, CSS, JS... client side stuff P3 Third level priority - Nice to have labels Aug 27, 2024
@janbrasna janbrasna changed the title Fix CTD promo & simplify landing Fixup CTD landing & update promo link Sep 28, 2024
@stephaniehobson
Copy link
Contributor

Needs a rebase 🙏

@janbrasna
Copy link
Contributor Author

(I cowardly did a merge instead to better show the diff, it should only be tabindex additions and data-cta-type removals from the recently landed PRs.)

Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

Wow, these are good changes and I'm sorry we left them to languish in the PR queue for so long. Many good simplifications and code deletions 💯

The only thing I think I would still carry over is the custom CTD social sharing image.

R+wc

{% block page_favicon_large %}{{ static('img/favicons/firefox/favicon-196x196.png') }}{% endblock %}
{% block page_ios_icon %}{{ static('img/favicons/firefox/apple-touch-icon.png') }}{% endblock %}
{% block page_title_suffix %} — {{ ftl('firefox-home-mozilla') }}{% endblock %}
{% block page_desc %}{{ seo_desc }}{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should bring the page_image over for CTD social share images.

Changing the template did make the favicon updates unnecessary though 🙂

Suggested change
{% block page_desc %}{{ seo_desc }}{% endblock %}
{% block page_desc %}{{ seo_desc }}{% endblock %}
{% block page_image %}
{% if LANG == "fr" %}
{{ static('img/firefox/challenge-the-default/ctd-share-fr.png') }}
{% else %}
{{ static('img/firefox/challenge-the-default/ctd-share.png') }}
{% endif %}
{% endblock %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no worries, it started out as a simple a11y +runaway tag fix, but while testing it and after validating further it turned out there's more to tackle — maybe if I submitted the individual issues separately, It would have been easier to review in smaller focused chunks, instead of this seemingly unrelated lump of tiny changes…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephaniehobson So this poster image thing is actually sorted out, just elsewhere:

{% extends "firefox/challenge-the-default/landing-base.html" %}
{% block page_image %}{{ static('img/firefox/challenge-the-default/ctd-share-fr.png') }}{% endblock %}

Every language sets it. It only needed removing this hardcoded logic in base to start working for all;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the three links I've chosen for testing above (de, pl, fr) should each render its own poster, as was originally intended in #14306/files additions.

NB: DE uses English branding, other languages have logos and such localized.

@stephaniehobson stephaniehobson removed the Needs Review Awaiting code review label Nov 15, 2024
Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

R+ 🚀

@stephaniehobson stephaniehobson merged commit b502710 into mozilla:main Nov 18, 2024
5 checks passed
@janbrasna janbrasna deleted the fix/ctd-woes branch November 18, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend HTML, CSS, JS... client side stuff P3 Third level priority - Nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants