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

editorial: Move the definition of display-mode back to APPMANIFEST. #1039

Merged
merged 6 commits into from
Apr 12, 2024
Merged

editorial: Move the definition of display-mode back to APPMANIFEST. #1039

merged 6 commits into from
Apr 12, 2024

Conversation

mgiuca
Copy link
Collaborator

@mgiuca mgiuca commented May 25, 2022 •
edited by pr-preview bot
Loading

This text was moved out of the Manifest spec into CSS mediaqueries-5 in
w3c/csswg-drafts#6343, along with the display-mode media feature. The
actual definition of display mode belongs here, while the display-mode
media feature remains in CSS mediaqueries-5.

Closes w3c/csswg-drafts#7306.

This change (choose at least one, delete ones that don't apply):

  • Makes editorial changes (changes informative sections, or changes normative sections without changing behavior)

Person merging, please make sure that commits are squashed with one of the following as a commit message prefix:

  • chore:
  • editorial:
  • BREAKING CHANGE:
  • And use none if it's a normative change

Preview | Diff

@mgiuca mgiuca requested a review from marcoscaceres May 25, 2022 06:13
@mgiuca mgiuca mentioned this pull request May 25, 2022
Closed
aarongustafson
aarongustafson approved these changes May 25, 2022
View reviewed changes
Copy link
Collaborator

@aarongustafson aarongustafson left a comment

Choose a reason for hiding this comment

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

LGTM

aarongustafson
aarongustafson requested changes May 25, 2022
View reviewed changes
Copy link
Collaborator

@aarongustafson aarongustafson left a comment

Choose a reason for hiding this comment

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

CSS will want the export, no?

index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres reviewed May 25, 2022
View reviewed changes
index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres reviewed May 25, 2022
View reviewed changes
index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres reviewed May 25, 2022
View reviewed changes
index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres reviewed May 25, 2022
View reviewed changes
index.html Outdated Show resolved Hide resolved
marcoscaceres
marcoscaceres approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Approved, but let's make sure we merge them both a the same time.

@mgiuca
Copy link
Collaborator Author

mgiuca commented May 26, 2022

Thanks for the export suggestions, @aarongustafson . I think you need to approve now that Marcos has merged it in.

Also @marcoscaceres do you know what is going on with this validation error? It doesn't appear to be a problem with the changes to the html file:

  The W3C_API_KEY input is required with VALIDATE_PUBRULES
  Error: Process completed with exit code 1.
  Error: w3c/spec-prod/v2/action.yml (Line: 83, Col: 27):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 83, Col: 27): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 93, Col: 23):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 93, Col: 23): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 113, Col: 24):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 113, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 122, Col: 33):
  Error: w3c/spec-prod/v2/action.yml (Line: 123, Col: 24):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 122, Col: 33): Error reading JToken from JsonReader. Path '', line 0, position 0.,w3c/spec-prod/v2/action.yml (Line: 123, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 132, Col: 35):
  Error: w3c/spec-prod/v2/action.yml (Line: 133, Col: 24):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 132, Col: 35): Error reading JToken from JsonReader. Path '', line 0, position 0.,w3c/spec-prod/v2/action.yml (Line: 133, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 143, Col: 33):
  Error: w3c/spec-prod/v2/action.yml (Line: 144, Col: 24):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 143, Col: 33): Error reading JToken from JsonReader. Path '', line 0, position 0.,w3c/spec-prod/v2/action.yml (Line: 144, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 153, Col: 24):
  Error: w3c/spec-prod/v2/action.yml (Line: 154, Col: 21):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 153, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.,w3c/spec-prod/v2/action.yml (Line: 154, Col: 21): Error reading JToken from JsonReader. Path '', line 0, position 0.
  Error: w3c/spec-prod/v2/action.yml (Line: 163, Col: 24):
  Error: w3c/spec-prod/v2/action.yml (Line: 164, Col: 24):
  Error: The template is not valid. w3c/spec-prod/v2/action.yml (Line: 163, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.,w3c/spec-prod/v2/action.yml (Line: 164, Col: 24): Error reading JToken from JsonReader. Path '', line 0, position 0.

@aarongustafson
Copy link
Collaborator

We think it's an issue at the W3C.

@aesinnllc

This comment was marked as off-topic.

Sign in to view
@marcoscaceres

This comment was marked as off-topic.

Sign in to view
@marcoscaceres
Copy link
Member

@mgiuca, I filed a bug for that error here: w3c/spec-prod#136

@aesinnllc

This comment was marked as off-topic.

Sign in to view
@marcoscaceres

This comment was marked as off-topic.

Sign in to view
@mgiuca mgiuca force-pushed the move-display-mode-back branch from 763f844 to 8f9e03a Compare November 4, 2022 02:48
@ewilligers
Copy link

Testing various browsers on a Mac, when a page has no manifest, and a browser tab is shown full-screen, browser controls remain visible but the menu bar is hidden. Chrome/Edge and Firefox treat this as fullscreen for Media Query purposes. Safari treats this as browser for Media Query purposes.

@mgiuca mgiuca force-pushed the move-display-mode-back branch from 8f9e03a to c6a7e09 Compare January 20, 2023 06:21
@mgiuca mgiuca force-pushed the move-display-mode-back branch from 0c6c5db to 5edcfed Compare April 21, 2023 05:17
@mgiuca mgiuca force-pushed the move-display-mode-back branch from 398f4ed to 49d61eb Compare November 16, 2023 06:03
@marcoscaceres
Copy link
Member

@mgiuca, feel free to merge it?

@mgiuca
Copy link
Collaborator Author

mgiuca commented Nov 20, 2023 •
edited
Loading

@marcoscaceres I haven't been merging this because I remember (>1 year ago.....) I got blocking comments out of band from @frivoal that I needed to action, and put this on the backburner. I'm trying to dig it up now (hence why I did a rebase) and finish this off.

Edit: Ah yes, here's the relevant request. It's pretty vague but I'll make some changes and ping to see if they're happy.

Edit: Here's the update to the PR on the MediaQueries side. No updates on the Manifest side.

@mgiuca mgiuca mentioned this pull request Nov 20, 2023
Closed
mgiuca and others added 4 commits February 26, 2024 16:19
@mgiuca
Move the definition of display-mode back to APPMANIFEST.
4d41b84 
Closes w3c/csswg-drafts#7306.

This text was moved out of the Manifest spec into CSS mediaqueries-5 in
w3c/csswg-drafts#6343, along with the display-mode media feature. The
actual definition of display mode belongs here, while the display-mode
media feature remains in CSS mediaqueries-5.
@marcoscaceres @aarongustafson @mgiuca
Apply suggestions from code review
b74f5ad 
Co-authored-by: Aaron Gustafson <[email protected]>
@mgiuca
Tidy.
b6b5223
@mgiuca
display-mode: Rewrite note about display-mode media feature to helpfu…
d49f0cf 
…lly point towards the CSS spec.

Also linkify display-mode properly.
@mgiuca mgiuca force-pushed the move-display-mode-back branch from 49d61eb to d49f0cf Compare February 26, 2024 05:35
@marcoscaceres
Copy link
Member

@mgiuca, let us know how/if/when we can proceed with merging this.

@mgiuca
Copy link
Collaborator Author

mgiuca commented Apr 11, 2024

Thanks for pinging the other PR. I would like to close this out but we should land both together.

I will try to engage CSS people again... if we literally can't get in touch with them we could maybe just land this or modify it to create a parallel definition ("manifest display mode") but it is not ideal to do so.

@marcoscaceres
Copy link
Member

@mgiuca I pinged a few more CSS folks behind the scenes to see how we get it merged. I’ll report back.

@marcoscaceres
Copy link
Member

(Worst case, I don’t mind if we make a small mess and just merge this if we don’t hear back)

@marcoscaceres marcoscaceres merged commit a791201 into w3c:main Apr 12, 2024
1 of 2 checks passed
@mgiuca
Copy link
Collaborator Author

mgiuca commented Apr 12, 2024

Thanks so much for following this up, @marcoscaceres . It's been a LONG time and I'm glad to have things finally set up properly.

@marcoscaceres
Copy link
Member

No problem. Glad I could be of some help.

@dmurph
Copy link
Collaborator

dmurph commented Apr 12, 2024

Yes, thanks a ton Marcos 👍

@mgiuca
Copy link
Collaborator Author

mgiuca commented Apr 29, 2024

@marcoscaceres is the WD version of the manifest spec supposed to update automatically? It hasn't for two weeks. From memory, it used to update automatically after about 24h but it seems stuck (it was last generated on April 10, right before this landed, but doesn't include this change).

@mgiuca mgiuca mentioned this pull request Apr 29, 2024
Open
@marcoscaceres
Copy link
Member

Will check what happened.

github-actions bot added a commit that referenced this pull request May 1, 2024
@mgiuca @github-actions
editorial: Move the definition of display-mode back to APPMANIFEST. (#…
98628c7 
…1039)

SHA: a791201
Reason: push, by marcoscaceres

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@marcoscaceres
Copy link
Member

Fixed... the publishing system was having issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcoscaceres marcoscaceres marcoscaceres approved these changes

@aarongustafson aarongustafson aarongustafson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[mediaqueries-5] Move the definition of "display mode" back to Manifest spec
6 participants
@mgiuca @aarongustafson @aesinnllc @marcoscaceres @ewilligers @dmurph