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

🐛 Adds microsoft-edge protocol #34168

Merged
merged 7 commits into from
Jan 6, 2022
Merged

🐛 Adds microsoft-edge protocol #34168

merged 7 commits into from
Jan 6, 2022

Conversation

PHCCorso
Copy link
Contributor

@PHCCorso PHCCorso commented May 3, 2021

Closes #34167

@CLAassistant
Copy link

CLAassistant commented May 3, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@amp-owners-bot
Copy link

amp-owners-bot bot commented May 3, 2021 •
edited by ampprojectbot
Loading

Hey @ampproject/wg-caching! These files were changed:

validator/validator-main.protoascii

Gregable
Gregable reviewed May 3, 2021
View reviewed changes
validator/validator-main.protoascii Outdated
@@ -1371,6 +1371,7 @@ tags: {
protocol: "web+mastodon" # See GitHub issue #14793
protocol: "wh"
protocol: "whatsapp"
protocol: "microsoft-edge"
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment with a link to:
https://docs.microsoft.com/en-us/windows/uwp/launch-resume/launch-default-app#microsoft-edge-uri-scheme

or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gregable Sure.

Added it 👍 .

Copy link
Member

Choose a reason for hiding this comment

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

Looks the validation entry needs to be sorted alphabetically as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanorozco done.

@Gregable Gregable requested review from alanorozco and removed request for alin04 May 4, 2021 16:34
@honeybadgerdontcare
Copy link
Contributor

@alanorozco could you triage or review, Will is no longer on AMP and unsure who owns amp-bind

alanorozco
alanorozco reviewed May 10, 2021
View reviewed changes
extensions/amp-bind/0.1/bind-validator.js Outdated
@@ -389,6 +389,7 @@ function createElementRules_() {
'web+mastodon': true,
'wh': true,
'whatsapp': true,
'microsoft-edge': true,
Copy link
Member

Choose a reason for hiding this comment

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

Please keep list alphabetically sorted below // 3rd Party Protocols.

microsoft-edge should go between line and skype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanorozco Yup, fixed. 👍

Copy link
Member

Choose a reason for hiding this comment

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

You need to sort it on this file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanorozco silly me. Totally overlooked that. Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanorozco ?

@alanorozco
Copy link
Member

Looks good to me except for sorting. I'll approve once that's addressed.

@alanorozco
Copy link
Member

@phcorso You need to sort it on bind-validator.js as well.

@alanorozco
Copy link
Member

alanorozco commented Oct 7, 2021 •
edited
Loading

@PHCCorso Apologies for the late response. Would love to merge this once able. Would you mind updating your branch?
https://app.circleci.com/pipelines/github/ampproject/amphtml/9428/workflows/f8e6c0f1-be51-49c3-92cc-4853b42f405c/jobs/144978

ERROR: pull/34168 is missing the latest CircleCI config revision 3592c8c0676f5e73e6b8ab1248a57119cff85d93.


This can be fixed in three ways:


1. Click the "Update branch" button on GitHub and follow instructions.
   ⤷ It can be found towards the bottom of the PR, after the list of checks.


2. Pull the latest commits from main and re-push the PR branch.
   ⤷ Follow these steps:

      git checkout main
      git pull
      git checkout <PR branch>
      git merge main
      git push origin


3. Rebase on main and re-push the PR branch.
   ⤷ Follow these steps:

      git checkout main
      git pull
      git checkout <PR branch>
      git rebase main
      git push origin --force



Exited with code exit status 1

@PHCCorso
Copy link
Contributor Author

@alanorozco done.

@alanorozco alanorozco merged commit da7957f into ampproject:main Jan 6, 2022
westonruter added a commit that referenced this pull request Jan 10, 2022
@westonruter
Merge branch 'main' of github.com:ampproject/amphtml into add/importa…
df9a488 
…nce-attr-to-hero-img

* 'main' of github.com:ampproject/amphtml: (525 commits)
  mathml storybook: supply missing component definition. (#37326)
  storybook: Iframe --> BentoIframe (#37327)
  🖍  [Story system layer] New ad badge (#37311)
  🐛 [amp story] Replay/next page button bug fix (#37316)
  🚀  [Story performance] Remove affiliate links (#37280)
  Compiler: Add amp-carousel-0.1 to the builder map (#37308)
  ⏪  [Story system layer] Audio icon disappears when story has background audio (#37314)
  🚀  [Story performance] Remove story access (#37281)
  Fix remapping esbuild output on Windows (#37312)
  🐛 adds in correct weight for amp-story-product-tag text (#37188)
  typechecking carousel: remove shame files (#37213)
  Use remapping to remap minified sourcemap into source code (#37238)
  SwG Release 0.1.22.199 (#37310)
  🐛 Adds microsoft-edge protocol (#34168)
  Sync for validator cpp engine and cpp htmlparser (#37292)
  ✨ amp-story-shopping Updated currency with product price and correct Localized currency (#37249)
  ✨[Smartadserver ad extension] Implement support for Fast Fetch (#36991)
  Remove client-side-experiments-config.json from this repo (#37304)
  🚮  Remove closure compiler logic from build-system. (#37296)
  🌐 Added RTL ordering i18n for amp story shopping tag (#37252)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gregable Gregable Gregable approved these changes

@alanorozco alanorozco alanorozco approved these changes

@dreamofabear dreamofabear Awaiting requested review from dreamofabear

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable WG: caching
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft Edge redirection mistakenly considered as HTTP protocol
7 participants
@PHCCorso @CLAassistant @honeybadgerdontcare @alanorozco @Gregable @twifkak @ampprojectbot