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.

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

Meta: Update style guide to discuss preferred orderings of elements #831

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

ben-allen
Copy link
Contributor

@ben-allen ben-allen commented Aug 31, 2023

@sffc sffc changed the title Update style guide to discuss preferred orderings of elements Meta: Update style guide to discuss preferred orderings of elements Sep 18, 2023
docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved

## Element Ordering

This section concerns the order in which containers store elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the definition of "container" in this context?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This section concerns the order in which containers store elements.
This section concerns the order in which properties are stored and accessed within plain objects.

Does this apply outside of plain objects?

docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
Comment on lines 11 to 29
- [Examples](#examples)
- [Alternative: Kebab case for all string enumerations](#alternative-kebab-case-for-all-string-enumerations)
- [Pros](#pros)
- [Cons](#cons)
- [Decision](#decision)
- [Alternative: Kebab case for new identifiers only](#alternative-kebab-case-for-new-identifiers-only)
- [Pros](#pros-1)
- [Cons](#cons-1)
- [Decision](#decision-1)
- [Alternative: Use kebab case but also accept camel case](#alternative-use-kebab-case-but-also-accept-camel-case)
- [Pros](#pros-2)
- [Cons](#cons-2)
- [Decision](#decision-2)
- [Identifiers defined outside ECMA-402](#identifiers-defined-outside-ecma-402)
- [Examples](#examples-1)
- [Alternative: Convert identifiers to camel case](#alternative-convert-identifiers-to-camel-case)
- [Pros](#pros-3)
- [Cons](#cons-3)
- [Decision](#decision-3)
Copy link
Member

Choose a reason for hiding this comment

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

Are the whitespace changes necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it back to the previous spacing

Copy link
Member

Choose a reason for hiding this comment

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

The previous spacing was 4 spaces and your new version has 2 spaces 🙈

@ben-allen ben-allen force-pushed the update-readme-note-on-ordering branch from 5a261af to 1ef3e6a Compare October 12, 2023 16:58
Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Some old comments and some new ones, let's clean this one up!


## Element Ordering

This section concerns the order in which containers store elements.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This section concerns the order in which containers store elements.
This section concerns the order in which properties are stored and accessed within plain objects.

Does this apply outside of plain objects?

docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
Comment on lines 11 to 29
- [Examples](#examples)
- [Alternative: Kebab case for all string enumerations](#alternative-kebab-case-for-all-string-enumerations)
- [Pros](#pros)
- [Cons](#cons)
- [Decision](#decision)
- [Alternative: Kebab case for new identifiers only](#alternative-kebab-case-for-new-identifiers-only)
- [Pros](#pros-1)
- [Cons](#cons-1)
- [Decision](#decision-1)
- [Alternative: Use kebab case but also accept camel case](#alternative-use-kebab-case-but-also-accept-camel-case)
- [Pros](#pros-2)
- [Cons](#cons-2)
- [Decision](#decision-2)
- [Identifiers defined outside ECMA-402](#identifiers-defined-outside-ecma-402)
- [Examples](#examples-1)
- [Alternative: Convert identifiers to camel case](#alternative-convert-identifiers-to-camel-case)
- [Pros](#pros-3)
- [Cons](#cons-3)
- [Decision](#decision-3)
Copy link
Member

Choose a reason for hiding this comment

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

The previous spacing was 4 spaces and your new version has 2 spaces 🙈

@ryzokuken ryzokuken added the meta label Feb 22, 2024
@ben-allen ben-allen force-pushed the update-readme-note-on-ordering branch 3 times, most recently from bafaa25 to 1a826c9 Compare May 4, 2024 17:31
@ben-allen ben-allen requested a review from gibson042 May 6, 2024 17:25
Copy link
Member

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks!

Comment on lines 220 to 225
:star2: <em>The `resolvedOptions` of Intl objects should appear in the following order:

1. `locale`
2. All properties (given in lexicographical order) that can be set by extension keys and that are guaranteed to exist
3. Properties (in lexicographical order) that are not set by extension keys and that are guaranteed to exist
4. All properties (in lexicographical order) that exist conditionally.</em> :star2:
Copy link
Member

Choose a reason for hiding this comment

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

Is this whole section purposefully italicized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to match the italicization in the other starred blocks. I do admit that italicizing the list might be a situation where the result of insisting on consistency is ugly enough that it's the wrong thing to do.

Copy link
Member

Choose a reason for hiding this comment

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

I think one way to do it would be to rephrase the first sentence into a summarized guideline, star+italicize it and follow that with the full list. An entire list being starred/italicized doesn't really say much and looks weird.

@ben-allen
Copy link
Contributor Author

@gibson042 what do you think about this version?

@ben-allen ben-allen force-pushed the update-readme-note-on-ordering branch from 2404d76 to 83a6734 Compare May 7, 2024 18:53
docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
docs/style-guide.md Outdated Show resolved Hide resolved
@ben-allen ben-allen force-pushed the update-readme-note-on-ordering branch from edc362a to ea71f4c Compare August 7, 2024 18:26
@ben-allen ben-allen force-pushed the update-readme-note-on-ordering branch from a833087 to 346a5c3 Compare August 7, 2024 18:39
@ben-allen
Copy link
Contributor Author

@gibson042 Does this look good to merge?

@gibson042 gibson042 merged commit 06c0232 into tc39:main Aug 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants