-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix(styles): design fixes for tool header [ci visual] #4688
Conversation
✅ Deploy Preview for fundamental-styles ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/styles/src/tool-header.scss
Outdated
@@ -68,6 +69,9 @@ $fd-tool-header-object-status-semantic-values: ( | |||
); | |||
|
|||
.#{$block} { | |||
// Paddings | |||
$fd-tool__header-item-spacing: 0.5rem !default; |
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.
for SCSS variables we only use -
. For example: $fd-tool-header-item-spacing
@@ -96,17 +96,19 @@ <h4>Size XL (3rem)</h4> | |||
</button> | |||
</div> | |||
</div> | |||
<div class="fd-tool-header__group"> | |||
<div class="fd-tool-header__group--center"> |
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 can't have a modifier class like fd-tool-header__group--center
without the base class fd-tool-header__group
. It should be:
fd-tool-header__group fd-tool-header__group--center
@@ -15,17 +15,20 @@ <h4 class="fd-title fd-title--h5 fd-tool-header__title">Product Name</h4> | |||
<label class="fd-text fd-tool-header__text" for="input-1">Description (Text Element)</label> | |||
</div> | |||
</div> | |||
<div class="fd-tool-header__group"> | |||
<div class="fd-tool-header__group--center"> |
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.
same as above comment
packages/styles/src/tool-header.scss
Outdated
&::before { | ||
// display: none; | ||
} |
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.
if not needed, please remove
packages/styles/src/tool-header.scss
Outdated
--fdSelect_Text_Content_Color: var(--fdShellbar_Select_Content_Color); | ||
--fdInputGroup_Input_Border: var(--fdShellbar_Input_Border); | ||
--fdInputGroup_Input_Color: var(--fdShellbar_Input_Color); | ||
--fdInputGroup_Input_Placeholder_Color: var(--fdShellbar_Input_Placeholder_Color); | ||
--fdInputGroup_Input_Placeholder_Style: var(--fdShellbar_Input_Placeholder_Style); | ||
--fdInputGroup_Background: none; | ||
--fdInputGroup_Background_Color: var(--fdShellbar_Input_Background); |
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.
all the variables starting with --fdShellbar....
should be duplicated for Tool Header, in my opinion. They should be moved to their own files and renamed to --fdToolHeader....
This will avoid the situation where the design for ShellBar changes but for Tool Header doesn't. By using the same CSS variables you are making Tool Header dependant on ShellBar, and they shouldn't be.
we need to update tool header with the updated design and the changes will be done there => closing the PR |
Related Issue
Relates to #3955 (comment)
Description
Updated the tool header classes to meet new requirements.
Added new class for search field input.
Screenshots
Before
After