Skip to content

Create a new page for CSS sign() function#5192

Merged
rachelandrew merged 9 commits intomdn:mainfrom
ramiy:patch-4
Sep 27, 2021
Merged

Create a new page for CSS sign() function#5192
rachelandrew merged 9 commits intomdn:mainfrom
ramiy:patch-4

Conversation

@ramiy
Copy link
Copy Markdown
Contributor

@ramiy ramiy commented May 21, 2021

What was wrong/why is this fix needed? (quick summary only)

Create a new MDN page for a new CSS function.

MDN URL of the main page changed

https://developer.mozilla.org/en-US/docs/Web/CSS/sign()

Issue number (if there is an associated issue)

No.

Anything else that could help us review it

sign() is a functional notation defined in CSS Values and Units Module Level 4.

See also: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Functions#math_functions

@ramiy ramiy requested a review from a team as a code owner May 21, 2021 15:43
@ramiy ramiy requested review from rachelandrew and removed request for a team May 21, 2021 15:43
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 21, 2021

Preview URLs

Flaws

URL: /en-US/docs/Web/CSS/sign()
Title: sign()
on GitHub
Flaw count: 2

  • macros:
    • /en-US/docs/Web/CSS/abs does not exist
  • bad_bcd_queries:
    • No BCD data for query: css.types.sign

External URLs

URL: /en-US/docs/Web/CSS/sign()
Title: sign()
on GitHub

No new external URLs

(this comment was updated 2021-09-19 08:00:49.372525)

@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented May 21, 2021

@rachelandrew

@rachelandrew
Copy link
Copy Markdown
Collaborator

@ramiy just a note, the BCD folk tend not to want completely unsupported things in BCD. We're trying to come up with a standard way to indicate "no support at all" without adding a BCD table here #5118

@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented May 31, 2021

@rachelandrew Examples updated. Please review.


<pre class="brush: css;">div {
background-position: sign(10%);
}</pre>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You are saying it might resolve one way or another, so I would suggest showing that in the example. As we don't have an implementation of this right now. So if you give this a container (and thus something to resolve against) you can give a clear example of what the value would resolve to.

If we have implementations wouldn't matter so much as people could see/check in DevTools but we need to be a bit more explicit with things that don't have a way to demo them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't have a better example. We can release the page as is, and let the community add better examples.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I explained above how to make the example better. At the moment it doesn't really demonstrate anything.

Give the element a container, with a width, then in the explanation to the example explain what the background position would be based on being sign(10%) of the container width. What would it resolve to?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rachelandrew can you insert a code suggestion to the PR?

Copy link
Copy Markdown
Collaborator

@rachelandrew rachelandrew left a comment

Choose a reason for hiding this comment

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

Couple of inline suggestions and comments

ramiy and others added 3 commits June 7, 2021 00:40
Co-authored-by: Rachel Andrew <me@rachelandrew.co.uk>
Co-authored-by: Rachel Andrew <me@rachelandrew.co.uk>
@sideshowbarker
Copy link
Copy Markdown
Member

@ramiy It seems maybe this is waiting on your response to the review comment at #5192 (review)

@teoli2003
Copy link
Copy Markdown
Contributor

I'm afraid this sat still for too long and, with the conversion of the CSS area to Markdown last week, this will need some extra work.

(I wonder why Github doesn't indicate any merge conflict, btw)

@teoli2003 teoli2003 added the Content:CSS Cascading Style Sheets docs label Sep 13, 2021
@sideshowbarker
Copy link
Copy Markdown
Member

I'm afraid this sat still for too long and, with the conversion of the CSS area to Markdown last week, this will need some extra work.

I think that’s just because it’s an entirely new file

@sideshowbarker
Copy link
Copy Markdown
Member

with the conversion of the CSS area to Markdown last week, this will need some extra work.

I went ahead and converted it to Markdown. But I’m not sure from who this PR is waiting on action from at this point…

@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented Sep 19, 2021

I think this PR is ready for merge. For new suggestions people can create other PRs.

@rachelandrew rachelandrew merged commit 615afc6 into mdn:main Sep 27, 2021
@ramiy ramiy deleted the patch-4 branch September 27, 2021 13:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Content:CSS Cascading Style Sheets docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants