feat(components/collapsiblepanel): style update #961
Conversation
charlesnguyen
changed the title
| } | ||
|
|
||
| &.muted { | ||
| color: $brand-muted; |
There was a problem hiding this comment.
You have created brand-muted on $grey.
I think just color $grey should be enough here.
$brand-xx are bootstrap-sass variable we have changed using our own colors in the bootstrap theme.
I will not block it for that but I would like other opinion
frassinier
changed the title
| @@ -254,13 +229,15 @@ CollapsiblePanel.propTypes = { | |||
| onSelect: PropTypes.func, | |||
| onToggle: PropTypes.func, | |||
| expanded: PropTypes.bool, | |||
| selected: PropTypes.bool, | |||
| status: PropTypes.string, | |||
There was a problem hiding this comment.
Breaking changes should be referenced into /BREAKING_CHANGES.MD file 🙏
| &.muted { | ||
| border-left: 4px solid $grey; | ||
| padding-left: 1px; | ||
| } |
There was a problem hiding this comment.
Don't Repeat Yourself
&.info,
&.success,
&.danger,
&.muted {
padding-left: 1px;
border-left: 4px solid;
}
&.info {
border-left-color: $brand-info;
}
&.success {
border-left-color: $brand-success;
}
&.danger {
border-left-color: $brand-danger;
}
&.muted {
border-left-color: $grey;
}
output/theme.sasslint.txt
Outdated
| 829:11 warning Hex notation should all be upper case hex-notation | ||
| 831:13 warning Hex notation should all be upper case hex-notation | ||
| 834:10 warning Hex notation should all be upper case hex-notation | ||
| 836:20 warning Hex notation should all be upper case hex-notation |
There was a problem hiding this comment.
Could you change sasslint configuration please?
1 similar comment
.sass-lint.yml
Outdated
| - 1 | ||
| - | ||
| style: 'uppercase' | ||
| - 0 |
There was a problem hiding this comment.
You should make a PR on the tools repository (https://github.com/Talend/tools/blob/master/tools-scss-formatting/.sass-lint.yml) prior to this change.
After that, please, make another PR for this change.
There was a problem hiding this comment.
This is be aligned with perttier ? In that case I auto agree with the PR on tools :D
| @@ -105,5 +97,3 @@ Status.propTypes = { | |||
| Status.defaultProps = { | |||
| actions: [], | |||
| }; | |||
|
|
|||
| export default Status; | |||
There was a problem hiding this comment.
Because of this, Status component is no more required like that :
import Status from 'blah';
but like that :
import { Status } from 'blah';
So I think this is a breaking change too. What do you think ?
| const adaptActions = actions.map(action => ({ | ||
| ...action, | ||
| onClick: getActionHandler(action.onClick, headerItem), | ||
| })); |
There was a problem hiding this comment.
adapt is meaningless. It doesn't describe the transformation applied to the actions. Please find a more descriptive name.
| [css.open]: expanded, | ||
| [css[getbsStyleFromStatus(status) || status]]: !!status, | ||
| status, | ||
| }); |
There was a problem hiding this comment.
Lots of complexity for some classes. I can't from a first read be sure of what makes it as class.
There was a problem hiding this comment.
@talend-glorieux I agree with you but I do not know how to improve it and I do not know why there were so much class.
| progress, | ||
| } = props; | ||
| export function Status(props) { | ||
| const { status, label, icon, actions, progress } = props; |
There was a problem hiding this comment.
Please put that spread inside the function arguments declaration. props isn't reused anywhere in the function.
output/theme.sasslint.txt
Outdated
| 828:11 warning Hex notation should all be upper case hex-notation | ||
| 830:13 warning Hex notation should all be upper case hex-notation | ||
| 833:10 warning Hex notation should all be upper case hex-notation | ||
| 835:20 warning Hex notation should all be upper case hex-notation |
There was a problem hiding this comment.
Please fix those errors.
| @@ -1,5 +1,21 @@ | |||
| # Breaking changes log | |||
|
|
|||
| ## v0.141.0 | |||
There was a problem hiding this comment.
What is the problem this PR is trying to solve?
New guidesline updates: https://company-57688.frontify.com/document/135457#/export-history-v2
Take into account the following changes:
-Left bar with the status color
-Status label must be displayed in black, not colored anymore
What is the chosen solution to this problem?
Please check if the PR fulfills these requirements
[x] This PR introduces a breaking change
-The boolean 'selected' used for 'selected CollapsiblePanel' is replaced by the string 'status' in order to customize the panel border
For Example:
selected: true => status = 'selected'
selected: false=> status = ''
-Status component has no longer default export.
import Status from '../Status'; ---> import { Status, getbsStyleFromStatus } from '../Status';