-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Theme generator: show in the website main nav #4251
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
Conversation
…c-react into show-theme-generator
| <div className={ css(pageStyles.u_maxTextWidth, styles.overview) }> | ||
| <h2 id='Overview'>Overview</h2> | ||
| <p>Fabric includes 9 theme colors and 11 neutral colors. Each has helper classes for text, background, border, and hover states. When selecting colors, refer to the <a className={ styles.colorsPageLink } href={ 'https://static2.sharepointonline.com/files/fabric/fabric-website/files/coloraccessibility_29sep2016.pdf' }>color accessibility guidance (PDF)</a> to ensure that your text can be ready by everyone.</p> | ||
| <p>Fabric includes 9 theme colors and 11 neutral colors. Each has helper classes for text, background, border, and hover states. When selecting colors, refer to the <a className={ styles.colorsPageLink } href={ 'https://static2.sharepointonline.com/files/fabric/fabric-website/files/coloraccessibility_29sep2016.pdf' }>color accessibility guidance (PDF)</a> to ensure that your text can be ready by everyone. If you need to customize your theme, see the <a className={ styles.colorsPageLink } href={ '#/styles/themegenerator' }>Theme generator</a>.</p> |
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.
Is this true anymore? I thought we removed all the helper classes.
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.
You tell me :)
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.
Yes, this blurb refers to Fabric Core which definitely still has these classes.
phkuo
left a comment
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.
Thanks!!
|
[ssr-tests 16:59:07] Starting: webpack |
Jahnp
left a comment
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.
Had one comment about leaving the URL for the brand-icons page for a later PR. Otherwise, everything looks great.
| { | ||
| title: 'Brand icons', | ||
| url: '#/styles/brand-icons', | ||
| url: '#/styles/brandicons', |
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.
Discussed in person--while it does indeed make sense for these to be consistent, doing this change will also break existing links. There may be a better way to handle a change/transition like this that won't result in a break--but we should probably look at doing that in a separate PR. That said--thanks for catching this!
Pull request checklist
$ npm run changeDescription of changes
Focus areas to test
(optional)