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

[Doc] Adds office color table to Design Tokens page #2073

Merged
merged 7 commits into from
May 20, 2024

Conversation

c0g1t8
Copy link
Contributor

@c0g1t8 c0g1t8 commented May 18, 2024

Pull Request

πŸ“– Description

A table at the end of the design tokens page appears to have been accidentally deleted.

[Edit] See comment Original delete intentional.

[Edit] Added new table based on review comments
image

🎫 Issues

  • missing table?

πŸ‘©β€πŸ’» Reviewer Notes

  • Was the table deletion intentional? If so, the text above it should be changed.
  • The text above the table also mentions accent colors for 17 different Office applications. I find it confusing and am unsure what it is trying to convey to the reader.

πŸ“‘ Test Plan

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

@c0g1t8 c0g1t8 marked this pull request as draft May 18, 2024 17:41
@c0g1t8 c0g1t8 changed the title [Doc] Accidentally deleted table? [Doc] Adds office color table to Design Tokens page May 19, 2024
@dvoituron
Copy link
Collaborator

Great!

Could you put a screenshot of the table in the description of this PR. This will be useful for the release notes.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented May 19, 2024

Great!

Could you put a screenshot of the table in the description of this PR. This will be useful for the release notes.

Done πŸ˜„

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented May 19, 2024

I fixed the visual glitch by hooking into OnContentConverted. MarkdownSection calls it multiple times. I think that is another issue that needs to be addressed separately.

Copy link
Contributor Author

@c0g1t8 c0g1t8 left a comment

Choose a reason for hiding this comment

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

Does Default count as an application?

@vnbaaij vnbaaij marked this pull request as ready for review May 19, 2024 18:31
@vnbaaij vnbaaij self-requested a review May 19, 2024 18:31
@vnbaaij
Copy link
Collaborator

vnbaaij commented May 19, 2024

What do you mean?

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented May 19, 2024

What do you mean?

There a 25 enumerated values for OfficeColor. Only 24 are "known" Microsoft applications.

Maybe, remove the actual number from the text above the table to remove any confusion. What do you think?

@vnbaaij
Copy link
Collaborator

vnbaaij commented May 19, 2024

What do you mean?

There a 25 enumerated values for OfficeColor. Only 24 are "known" Microsoft applications.

Maybe, remove the actual number from the text above the table to remove any confusion. What do you think?

Yes, that sounds good to me.

@vnbaaij vnbaaij enabled auto-merge (squash) May 20, 2024 05:28
@vnbaaij vnbaaij merged commit f4a9e93 into microsoft:dev May 20, 2024
2 checks passed
@c0g1t8 c0g1t8 deleted the fix_design branch May 24, 2024 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants