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

Position anchor #196

Merged
merged 19 commits into from
Jun 25, 2024
Merged

Position anchor #196

merged 19 commits into from
Jun 25, 2024

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented May 23, 2024 •
edited
Loading

Description

Implements the position-anchor property.

Related Issue(s)

#179

Steps to test/reproduce

View the demo in a browser that needs the polyfill, and apply the polyfill.

Show me

image
  • Update Tests
  • Confirm CSSTree parsing .class#id as .class #id is not an internal bug
  • Support auto value (defer until later)

@jamesnw jamesnw requested a review from jgerigmeyer May 23, 2024 12:53
@netlify Netlify
Copy link

netlify bot commented May 23, 2024 •
edited
Loading

✅ Deploy Preview for anchor-polyfill ready!

Name Link
🔨 Latest commit e205a22
🔍 Latest deploy log https://app.netlify.com/sites/anchor-polyfill/deploys/6679d981125f3500087997ea
😎 Deploy Preview https://deploy-preview-196--anchor-polyfill.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify Netlify
Copy link

netlify bot commented May 23, 2024 •
edited
Loading

✅ Deploy Preview for anchor-position-wpt canceled.

Name Link
🔨 Latest commit e205a22
🔍 Latest deploy log https://app.netlify.com/sites/anchor-position-wpt/deploys/6679d9815239330008ee6ae8

@jamesnw jamesnw linked an issue May 23, 2024 that may be closed by this pull request
Default anchor: position-anchor #179
Closed
jamesnw
jamesnw commented May 23, 2024
View reviewed changes
src/parse.ts Outdated Show resolved Hide resolved
jamesnw
jamesnw commented May 23, 2024
View reviewed changes
src/parse.ts Outdated Show resolved Hide resolved
jamesnw
jamesnw commented May 23, 2024
View reviewed changes
src/parse.ts Outdated Show resolved Hide resolved
jgerigmeyer
jgerigmeyer requested changes May 24, 2024
View reviewed changes
Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

@jamesnw I'm slowly trying to re-learn this codebase, so forgive me if my concern here is completely off! 😅

src/parse.ts Outdated Show resolved Hide resolved
@jamesnw jamesnw requested a review from jgerigmeyer June 10, 2024 17:58
@jamesnw
Copy link
Contributor Author

jamesnw commented Jun 10, 2024

@jgerigmeyer This is ready for re-review at your convenience.

I did opt to leave out support position-anchor: auto, which is "equivalent to omitting the anchor element in anchor()", as implicit anchors are still pending in the spec, it isn't tested yet in WPT, and it isn't implemented yet in Chrome.

@jamesnw
Copy link
Contributor Author

jamesnw commented Jun 10, 2024

This does introduce a new regression in anchor-position-multicol-colspan-001.html, .target 3. This is related to #192.

This change passes 12 new subtests, mostly in areas where position-anchor is not the primary concept under test.

jamesnw
jamesnw commented Jun 17, 2024
View reviewed changes
src/parse.ts Outdated Show resolved Hide resolved
@jgerigmeyer
Merge branch 'main' into position-anchor
2299533 
* main:
  chore(deps-dev): Bump the dev group across 1 directory with 11 updates
  chore(deps): Bump @types/css-tree from 2.3.7 to 2.3.8 in the prod group
  no more eslint updates
  chore(deps-dev): Bump ws from 8.17.0 to 8.17.1 in the npm_and_yarn group
@jgerigmeyer
update comment
c6ae11a
@jgerigmeyer
consistent type imports
f8ee261
@jgerigmeyer
remove duplicate fn
ac3370d
@jgerigmeyer
use generateCSS fn
fd93c65
jgerigmeyer
jgerigmeyer reviewed Jun 21, 2024
View reviewed changes
Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

This is looking great! A few remaining questions.

public/position-anchor.css Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
src/cascade.ts Outdated Show resolved Hide resolved
src/cascade.ts Outdated Show resolved Hide resolved
src/parse.ts Outdated Show resolved Hide resolved
src/parse.ts Outdated Show resolved Hide resolved
src/cascade.ts Outdated Show resolved Hide resolved
src/cascade.ts Outdated Show resolved Hide resolved
jgerigmeyer
jgerigmeyer approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

🚀

tests/unit/setup.ts Outdated Show resolved Hide resolved
@jgerigmeyer
Copy link
Member

@jamesnw I'm happy for you to merge this if you don't have more changes!

@jamesnw jamesnw merged commit 8af0367 into main Jun 25, 2024
12 checks passed
@jamesnw jamesnw deleted the position-anchor branch June 25, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgerigmeyer jgerigmeyer jgerigmeyer 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.

Default anchor: position-anchor
2 participants
@jamesnw @jgerigmeyer