Conversation
|
Preview: https://patternfly-pr-6199.surge.sh A11y report: https://patternfly-pr-6199-a11y.surge.sh |
44b5b99 to
e5a5121
Compare
12d8764 to
4b0eb93
Compare
|
Renamed vars, cleaned up padding on header-content, added padding to code block |
191e51d to
29bd9d8
Compare
There was a problem hiding this comment.
LGTM! Left a couple of comments, neither are blockers. If you agree anything should change, we can open a follow-up if issue for post-alpha you'd prefer.
8000
| border-radius: var(--#{$code-editor}__tab--BorderRadius) 0 0 0; | ||
|
|
||
| &.pf-m-plain { | ||
| --#{$code-editor}__header-content--BackgroundColor: var( --#{$code-editor}__header-content--m-plain--BackgroundColor); |
There was a problem hiding this comment.
small nit.
| --#{$code-editor}__header-content--BackgroundColor: var( --#{$code-editor}__header-content--m-plain--BackgroundColor); | |
| --#{$code-editor}__header-content--BackgroundColor: var(--#{$code-editor}__header-content--m-plain--BackgroundColor); |
Also I wonder if this .pf-m-plain modifier might be better on .#{$code-editor}__header in case we add more "plain" styles or do something like move the background color from .#{$code-editor}__header-content and add it to .#{$code-editor}__header? Not a blocker or anything though.
There was a problem hiding this comment.
I moved the plain modifier up to __header. I think the background color needs to remain on the header-content though, because moving it up to the header was creating a grey background when it shouldn't (like when there's no header only the language tab).
| background-color: var(--#{$code-editor}__main--BackgroundColor); | ||
| border: var(--#{$code-editor}__main--BorderWidth) solid; | ||
| border-color: var(--#{$code-editor}__main--BorderColor); | ||
| border-radius: 0 0 var(--#{$code-editor}__tab--BorderRadius) var(--#{$code-editor}__tab--BorderRadius); |
There was a problem hiding this comment.
Looks like we have __tab--BorderRadius defined on __main, __header-content, and __tab. That means to change any of them, a user will set --#{$code-editor}__tab--BorderRadius.
If we decide to give main (as an example) a unique border-radius via --#{$code-editor}__main--BorderRadius, then --#{$code-editor}__tab--BorderRadius will no longer control __main's border-radius. If a user themed --#{$code-editor}__tab--BorderRadius, that will no longer style __main, which is technically a breaking change.
That logic and what we consider a breaking change may change as we nail down the definition of what what a breaking change is in core now that we have such a rich set of vars to style everything, but for now would you mind creating unique vars for __main, __header-content, and __tab?
There was a problem hiding this comment.
Sure I'll separate them out. I think the reason I went with a single variable was because conceptually the 3 create the rounded edges on the editor header together.
|
🎉 This PR is included in version 6.0.0-alpha.77 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Todos/questions -
Header controls look a little off when updated to icon buttons. I put in a couple overrides but it's not quite there. Adjusting the padding/flex properties to match Figma better causes some weird alignment issues with the backgrounds/border. It also seems like we may still need hover styling for the icon buttons?View shortcuts button hover is covering/removing the header borderUpdate: should be matching figma.