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.

Sign up for GitHub

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

Jump to bottom

HeroDevs support #468

Merged
merged 2 commits into from
Oct 12, 2024
Merged

HeroDevs support #468

merged 2 commits into from
Oct 12, 2024

Conversation

timmywil
Copy link
Member

@timmywil timmywil commented Sep 29, 2024 •
edited
Loading

I made some modifications to #467, including:

  • generalized banner for future use
  • set banner message explicitly in post meta (jquery.com's index lives in its repo, so we'll set that there). I set api.jquery.com's banner in its theme's index page. Before, the banner was showing on the homepage of all jQuery wordpress sites (e.g. jquerymobile.com), but we only want it to show on site specific to jQuery core (i.e. jquery.com and api.jquery.com).
  • adjusted styles for version support warnings. used the fontawesome icon we are already loading instead of new svg.

Ref gh-467
Closes gh-462

See companion PRs for jquery.com and api.jquery.com.

@timmywil timmywil requested review from Krinkle and mgol and removed request for Krinkle September 29, 2024 23:07
This was referenced Sep 29, 2024
Closed
Merged
timmywil added a commit to timmywil/jquery.com that referenced this pull request Sep 30, 2024
@timmywil
All: improve visibility of support options
e05ccfd 
- styles for the new download button are in jquery/jquery-wp-content#468

Ref jquery/jquery-wp-content#462
timmywil added a commit to timmywil/jquery.com that referenced this pull request Sep 30, 2024
@timmywil @AndreAngelantoni
All: improve visibility of support options
c019d39 
- styles for the new download button are in jquery/jquery-wp-content#468

Ref jquery/jquery-wp-content#462
Ref jquerygh-245
Closes jquerygh-243

Co-authored-by: Andre Angelantoni <[email protected]>
@timmywil timmywil mentioned this pull request Sep 30, 2024
Merged
Krinkle
Krinkle requested changes Oct 1, 2024
View reviewed changes
themes/api.jquery.com/index.php Outdated Show resolved Hide resolved
@joeeames
Copy link

joeeames commented Oct 2, 2024

I believe that was a change @timmywil made after Andre prepared the visual changes? Can you weigh in here?

mgol
mgol reviewed Oct 6, 2024
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

One question

themes/api.jquery.com/index.php Outdated Show resolved Hide resolved
@timmywil timmywil mentioned this pull request Oct 8, 2024
Merged
@timmywil timmywil requested review from Krinkle and mgol October 8, 2024 03:07
@Krinkle
Copy link
Member

Krinkle commented Oct 9, 2024

It looks like there might be a recursive/bi-directional dependency on the jquery.com PR because as-is this breaks the home page as follows for me locally:

Screenshot

If we don't care about a few minutes of corruption, one way to deploy this is to first merge jquery-wp-content (breaks homepage), then the jquery.com PR (and wait for it to update prod), and strictly after that quickly push a commit to jquery-wp-content that bumps the stylesheet version in header.php. Then, after ~5min of Cloudflare cache expiry, everyone should see the new HTML and new CSS.

Alternatively, if it's feasible, perhaps we can make sure the HTML/CSS aren't mutually exclusive within this patch.

@timmywil
Copy link
Member Author

timmywil commented Oct 9, 2024

Indeed. It'd be ideal if both PRs could be released at the same time.

@timmywil
Copy link
Member Author

I've changed the styles so that the new download section is styled separately from the old one to avoid the circular dependency. Now, we can merge this PR first and it won't break anything. Then merge the jquery.com PR. Then we can remove the old styles whenever.

Krinkle
Krinkle reviewed Oct 11, 2024
View reviewed changes
Copy link
Member

@Krinkle Krinkle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Now that the patch is independent, you'll want to bump the base.css and stylesheet_url versions in header.php within this patch, so that the new styles will be there (instead of the old cached version), when you merge the jquery.com PR after this.

@Krinkle Krinkle mentioned this pull request Oct 11, 2024
Closed
mgol
mgol approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

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

LGTM, but also what Timo said - please update the query param for base.css.

Krinkle
Krinkle approved these changes Oct 12, 2024
View reviewed changes
Copy link
Member

@Krinkle Krinkle left a comment

Choose a reason for hiding this comment

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

Tested locally, both with and without the jquery.com PR on top. LGTM.

@timmywil timmywil merged commit 2015fcb into jquery:main Oct 12, 2024
5 checks passed
timmywil pushed a commit that referenced this pull request Oct 12, 2024
@AndreAngelantoni @timmywil
All: add banner, version support link, new download button styles
6d4c536 
Closes gh-468
@timmywil timmywil deleted the hero_devs branch October 12, 2024 13:36
Krinkle added a commit that referenced this pull request Oct 12, 2024
@Krinkle
All: add jq_url_append_version()
06069fe 
* Switch from central list to per-url hash.
  This removes the need to know about and remember updating a
  central list. I noticed one file missing already, namely the
  typesense-minibar.js file.

  This slightly improves cache use by not invalidating them
  all together.

* The filemtime timestamp values are so short that an MD5 hash of three
  filemtime() values is actually longer than just the three numbers
  concatenated. This means we lack the benefit a hash usually provides,
  which is that you get exposure to high entropy input in a format
  shorter than that same input.

* Switch from mtime (or mtime-hash) to content-hash.
  This has a few benefits:
  - Note that Git does not track modified times.
    Instead, mtime is whenever a `git clone`, `checkout` or `pull`
    last created or modified the file locally on that server. For
    example, if you `git checkout OLD` and then back to
    `git checkout main`, the files get a newer mtime. This is what
    happens by default at the OS level. Git isn't setting these
    mtime values.

    This means the URL ends up alternating depending on which server
    (e.g. wp-04 and wp-05) last generated the page. It also means
    different pages on the site may have a different URLs for the
    same asset, thus making ineffective use of caching. If the site
    was statically generated in CI, it would bump the cache after every
    commit instead of only when that file has changed, because in CI
    the "git clone" woulld create/modify all files at that time.

  - It is the same between local, staging, and production, which
    might ease debugging in some cases.

  - It allows continuing and re-using old browser (and CDN) caches
    if a commit is rolled back for some reason, since it would
    literally be the same content and thus URL again.

Follows-up 2015fcb.

Ref #469
Ref #468
@Krinkle Krinkle mentioned this pull request Oct 12, 2024
Merged
Krinkle added a commit that referenced this pull request Oct 12, 2024
@Krinkle
All: add jq_url_append_version()
03f6960 
* Switch from central list to per-url hash.
  This removes the need to know about and remember updating a
  central list. I noticed one file missing already, namely the
  typesense-minibar.js file.

  This slightly improves cache use by not invalidating them
  all together.

* The filemtime timestamp values are so short that an MD5 hash of three
  filemtime() values is actually longer than just the three numbers
  concatenated. This means we lack the benefit a hash usually provides,
  which is that you get exposure to high entropy input in a format
  shorter than that same input.

* Switch from mtime (or mtime-hash) to content-hash.
  This has a few benefits:
  - Note that Git does not track modified times.
    Instead, mtime is whenever a `git clone`, `checkout` or `pull`
    last created or modified the file locally on that server. For
    example, if you `git checkout OLD` and then back to
    `git checkout main`, the files get a newer mtime. This is what
    happens by default at the OS level. Git isn't setting these
    mtime values.

    This means the URL ends up alternating depending on which server
    (e.g. wp-04 and wp-05) last generated the page. It also means
    different pages on the site may have a different URLs for the
    same asset, thus making ineffective use of caching. If the site
    was statically generated in CI, it would bump the cache after every
    commit instead of only when that file has changed, because in CI
    the "git clone" woulld create/modify all files at that time.

  - It is the same between local, staging, and production, which
    might ease debugging in some cases.

  - It allows continuing and re-using old browser (and CDN) caches
    if a commit is rolled back for some reason, since it would
    literally be the same content and thus URL again.

Follows-up 2015fcb.

Ref #469
Ref #468
timmywil added a commit to jquery/api.jquery.com that referenced this pull request Oct 13, 2024
@timmywil
Categories: add version support warnings to old versions
85ed300 
Ref jquery/jquery-wp-content#468
Ref jquery/jquery.com#246
Closes gh-1251
timmywil added a commit to jquery/infrastructure-puppet that referenced this pull request Oct 13, 2024
@timmywil
wordpress: set jquery.com and api.jquery.com banner via php env var
a38b2bd 
Ref jquery/jquery-wp-content#468
Ref jquery/jquery.com#246
Ref jquery/api.jquery.com#1251
Closes gh-64
Krinkle added a commit that referenced this pull request Oct 13, 2024
@Krinkle @timmywil
All: add jq_url_append_version() (#470)
7273897 
* Switch from central list to per-url hash.
  This removes the need to know about and remember updating a
  central list. I noticed one file missing already, namely the
  typesense-minibar.js file.

  This slightly improves cache use by not invalidating them
  all together.

* The filemtime timestamp values are so short that an MD5 hash of three
  filemtime() values is actually longer than just the three numbers
  concatenated. This means we lack the benefit a hash usually provides,
  which is that you get exposure to high entropy input in a format
  shorter than that same input.

* Switch from mtime (or mtime-hash) to content-hash.
  This has a few benefits:
  - Note that Git does not track modified times.
    Instead, mtime is whenever a `git clone`, `checkout` or `pull`
    last created or modified the file locally on that server. For
    example, if you `git checkout OLD` and then back to
    `git checkout main`, the files get a newer mtime. This is what
    happens by default at the OS level. Git isn't setting these
    mtime values.

    This means the URL ends up alternating depending on which server
    (e.g. wp-04 and wp-05) last generated the page. It also means
    different pages on the site may have a different URLs for the
    same asset, thus making ineffective use of caching. If the site
    was statically generated in CI, it would bump the cache after every
    commit instead of only when that file has changed, because in CI
    the "git clone" woulld create/modify all files at that time.

  - It is the same between local, staging, and production, which
    might ease debugging in some cases.

  - It allows continuing and re-using old browser (and CDN) caches
    if a commit is rolled back for some reason, since it would
    literally be the same content and thus URL again.

Follows-up 2015fcb.

Ref #469
Ref #468

Co-authored-by: Timmy Willison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgol mgol mgol approved these changes

@Krinkle Krinkle Krinkle approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add HeroDevs support messaging for jQuery
5 participants
@timmywil @joeeames @Krinkle @mgol @AndreAngelantoni