Skip to content

<calc-constant> - New CSS data type#20856

Merged
wbamberg merged 39 commits intomdn:mainfrom
ramiy:patch-5
Oct 25, 2022
Merged

<calc-constant> - New CSS data type#20856
wbamberg merged 39 commits intomdn:mainfrom
ramiy:patch-5

Conversation

@ramiy
Copy link
Copy Markdown
Contributor

@ramiy ramiy commented Sep 18, 2022 •
edited
Loading

Description

Document the CSS <calc-constant> data type.

Motivation

This data type will help us document the new CSS constants - e | pi | infinity | -infinity | NaN

Related issues and pull requests

mdn/browser-compat-data#17828

@ramiy ramiy requested a review from a team as a code owner September 18, 2022 12:19
@ramiy ramiy requested review from estelle and removed request for a team September 18, 2022 12:19
@github-actions github-actions bot added the Content:CSS Cascading Style Sheets docs label Sep 18, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 18, 2022 •
edited
Loading

Preview URLs

Flaws (3)

URL: /en-US/docs/Web/CSS/calc-constant
Title: <calc-constant>
Flaw count: 3

  • macros:
    • /en-US/docs/Web/CSS/calc-sum does not exist
    • /en-US/docs/Web/CSS/calc-product does not exist
    • /en-US/docs/Web/CSS/calc-value does not exist

(this comment was updated 2022-10-25 21:48:51.732419)

@ramiy ramiy mentioned this pull request Sep 18, 2022
Merged
bsmth
bsmth reviewed Sep 20, 2022
View reviewed changes
@ramiy @bsmth
Update files/en-us/web/css/calc-constant/index.md
bf9ee55 
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
@wbamberg
Copy link
Copy Markdown
Collaborator

wbamberg commented Sep 20, 2022 •
edited
Loading

You can include a formal syntax section using something like:

## Formal syntax

{{csssyntax}}

(see, for example, http://localhost:5042/en-US/docs/Web/CSS/alpha-value.)

@ramiy ramiy mentioned this pull request Sep 21, 2022
Closed
wbamberg
wbamberg reviewed Sep 28, 2022
View reviewed changes
wbamberg
wbamberg requested changes Oct 6, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, @ramiy ! I wasn't sure whether @bsmth was on this one, but since it's been quiet for a little while...

I think this page needs an H3 "Values" immediately before the H3 "Formal syntax". This "Values" should be a Markdown <dl> listing all the values this type can be, in alphabetical order.

I think that would make this bit:

- `e` is the base of the natural logarithm, approximately equal to `2.7182818284590452354`.
- `pi` is the ratio of a circle's circumference to its diameter, approximately equal to `3.1415926535897932`.

...redundant.

I also think all of the bit at the start, from "Rather than" to the H2 "Syntax", should go in an H2 "Description". But see also my comment on that section: we don't just want to copy/paste the spec into MDN.

estelle
estelle reviewed Oct 6, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

A few suggestions to reduce the reading level required and make clearer that NaN has to be written case exact.

@estelle
Copy link
Copy Markdown
Member

estelle commented Oct 6, 2022

Should we include a like to the definitions of pi, infinity, e, and NaN, either in the content or in the see also?

@ramiy @estelle
Apply suggestions from code review
0052c1a 
Co-authored-by: Estelle Weyl <estelle@openwebdocs.org>
@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented Oct 6, 2022

Should we include a like to the definitions of pi, infinity, e, and NaN, either in the content or in the see also?

In other math functions we linked to Wikipedia definitions, not in "see also" section.

@ramiy ramiy requested a review from wbamberg October 13, 2022 12:29
@bsmth bsmth mentioned this pull request Oct 13, 2022
Closed
12 tasks
wbamberg
wbamberg requested changes Oct 13, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you @ramiy . This looks good, but I had a few comments here and there.

@ramiy @wbamberg
Apply suggestions from code review
dacc607 
Co-authored-by: wbamberg <will@bootbonnet.ca>
@bsmth bsmth requested a review from wbamberg October 24, 2022 11:08
@bsmth
Copy link
Copy Markdown
Member

bsmth commented Oct 24, 2022

@wbamberg would you mind taking another look?

@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented Oct 24, 2022

@bsmth I added your changes to this PR and then added few more stuff. You ran over them...

@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented Oct 24, 2022 •
edited
Loading

@bsmth Please use GitHub suggestions. Let's discuss them here before merging to the PR.

@bsmth
Copy link
Copy Markdown
Member

bsmth commented Oct 24, 2022

Hi @ramiy, myself and Will are in agreement about removing the section headings. Please refer to the template page structure for documenting CSS properties under our writing guidelines.

The changes required for this PR to land are that the headings should be removed and the content under these headings should be incorporated elsewhere in the document.

  • "Where constants can be used?" - can be under syntax
  • "Returned data type"- they do not have a return type, but a hint for <number> types can be added to the intro
  • "Case sensitivity" - can be under syntax

For a hint on making these changes, have a look at my previous commit.

@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented Oct 24, 2022

Moving those sections to the intro / syntax mixes different aspects.

I think it will be better to leave all the content in "Description", removing all the H3 headings - just like Will initially suggested.

@bsmth
Copy link
Copy Markdown
Member

bsmth commented Oct 24, 2022

Hi Ramiy,

leave all the content in "Description", removing all the H3 headings

Yes that would help us to get your changes merged, thank you

bsmth
bsmth approved these changes Oct 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thanks Ramiy 👍🏻

@wbamberg do you want to have another look?

wbamberg
wbamberg reviewed Oct 24, 2022
View reviewed changes
wbamberg
wbamberg reviewed Oct 24, 2022
View reviewed changes
wbamberg
wbamberg reviewed Oct 24, 2022
View reviewed changes
wbamberg
wbamberg reviewed Oct 25, 2022
View reviewed changes
@ramiy
Copy link
Copy Markdown
Contributor Author

ramiy commented Oct 25, 2022

Ok, looks good. Let's merge.

wbamberg
wbamberg approved these changes Oct 25, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @ramiy !

@wbamberg wbamberg merged commit da86cab into mdn:main Oct 25, 2022
@bsmth
Copy link
Copy Markdown
Member

bsmth commented Oct 26, 2022

Thanks all for the contribution & reviews

@ramiy ramiy deleted the patch-5 branch October 26, 2022 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estelle estelle estelle left review comments

@wbamberg wbamberg wbamberg approved these changes

@bsmth bsmth bsmth approved these changes

Assignees

No one assigned

Labels

Content:CSS Cascading Style Sheets docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ramiy @wbamberg @estelle @bsmth