-
Notifications
You must be signed in to change notification settings - Fork 160
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
chore: refactor and export scope terms #882
Conversation
53b0e2f
to
7574af0
Compare
(FYI, I'm signing off for the weekend. We'll continue the reviews on Monday! Have a good weekend.) |
@mgiuca, I think I've addressed all your feedback... hopefully all good to go! |
Ok, hopefully all good now @mgiuca? 🤞🤞🥺🤞🤞 |
Hrm ... pr-preview doesn't seem to have been updated. It's still stuck on 3ed7528. (Come to think of it ... that's very old, it's plausible that my previous round of reviews I was reviewing an older version than what you had presented..... that might explain why I thought you didn't do some of my suggestions. If so, I'm very sorry.) |
7cf57d8
to
23ad610
Compare
Oh man, that's my fault! argh, how embarrassing. Seems I didn't push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I've dropped a bunch of comments that are mostly nits. Happy with how this ended up. Please take into account my comments, then merge.
I do suspect my previous review was based on an old version. :| sorry.
It can't be that you forgot to push. pr-preview hasn't updated since June 11.
Me too! Thanks for all your help. It was a long slog, but this ended up 100 times better! |
Great, thanks @marcoscaceres ! |
This change (choose one):
changes normative sections without changing behavior)
Commit message:
Export "manifest/within scope" and "URL/within scope".
Preview | Diff