Restored Text widget expand functionality with display fixes #206
Conversation
marcin-witaszek-wttech
requested review from
goeson00,
Southeaven and
michaldziubek
| @@ -56,3 +59,11 @@ export const renderCardContent = ( | |||
| </StyledCardContent> | |||
| ); | |||
| }; | |||
|
|
|||
| const AlwaysVisibleContent = ({ type, status, content, expanded }) => { | |||
There was a problem hiding this comment.
I am not sure if AlwaysVisibleContent is the most fortunate name regarding the fact it can return null. Maybe sth like ExpandableContent or ComponentSpecificAdditionalContent/AdditionalContent?
There was a problem hiding this comment.
I don't like the name as well :) What about the PrimaryContent? Anyway this logic for displaying content starting to cause me headache... Maybe we should think about serious restructure? We should think about all the content we have in the widget and decide which should be managed on the widget level and on the widget content level.
There was a problem hiding this comment.
Good point, I choose ExpandableContent
There was a problem hiding this comment.
Ok, let's catch up after conference to discuss the approach. I agree that it needs some abstraction refactor.
| @@ -56,3 +59,11 @@ export const renderCardContent = ( | |||
| </StyledCardContent> | |||
| ); | |||
| }; | |||
|
|
|||
| const ExpandableContent = ({ type, status, content, expanded }) => { | |||
| return type !== 'TextWidget' ? ( | |||
There was a problem hiding this comment.
Im not sure about this complex ternary, it is extremely hard to read. How about simplifying this? Could be in some separate ticket if there is no time for that now.
There was a problem hiding this comment.
Ok, I will refactor it in a moment
There was a problem hiding this comment.
done
| @@ -56,3 +59,11 @@ export const renderCardContent = ( | |||
| </StyledCardContent> | |||
| ); | |||
| }; | |||
|
|
|||
| const AlwaysVisibleContent = ({ type, status, content, expanded }) => { | |||
There was a problem hiding this comment.
I don't like the name as well :) What about the PrimaryContent? Anyway this logic for displaying content starting to cause me headache... Maybe we should think about serious restructure? We should think about all the content we have in the widget and decide which should be managed on the widget level and on the widget content level.
| return type !== 'TextWidget' ? ( | ||
| <WidgetTypeIcon type={type} status={status} content={content} /> | ||
| ) : expanded ? null : ( | ||
| <TextWidget {...content} singleLine={true} /> | ||
| ); |
There was a problem hiding this comment.
Try to avoid such complicated ternary logic. It would be much nicer with ifs.
There was a problem hiding this comment.
Good point, done
| @@ -54,7 +64,8 @@ const TextWidget = ({ text, textSize, isVertical }) => { | |||
| TextWidget.propTypes = { | |||
| text: string.isRequired, | |||
| textSize: string.isRequired, | |||
| isVertical: bool.isRequired | |||
| isVertical: bool.isRequired, | |||
| singleLine: bool | |||
There was a problem hiding this comment.
Provide default for non required prop.
There was a problem hiding this comment.
Done
No description provided.