-
Notifications
You must be signed in to change notification settings - Fork 14
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
EZP-29746: Selected item blinking while loading its children in UDW #111
Conversation
locationsMap: this.locationsMap, | ||
activeLocations: this.activeLocations, | ||
}), | ||
() => ({ locationsMap: this.locationsMap, activeLocations: this.activeLocations }), |
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 you created setPreselectedState
method you can use it here.
return null; | ||
} | ||
|
||
return ( | ||
<SelectContentButtonComponent | ||
multiple={multiple} | ||
selectedContent={selectedContent} | ||
isSelected={selectedContent.find((content) => content.id === location.id)} |
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.
isSelected
suggests it's boolean but is passed content?
ping @dew326 |
@@ -69,13 +61,14 @@ export default class FinderTreeBranchComponent extends Component { | |||
const contentTypeHref = location.ContentInfo.Content.ContentType._href; | |||
const isContainer = contentTypesMap && contentTypesMap[contentTypeHref] && contentTypesMap[contentTypeHref].isContainer; | |||
const isSelectable = !(this.props.allowContainersOnly && !isContainer); | |||
const selected = location.id === this.state.selectedItem; |
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.
I would go for selectedLocationId
instead of selectedItem
because for the former is more descriptive.
isLocationAllowed, | ||
isPreviewMetaReady: false, | ||
})); | ||
this.setState(() => ({ |
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.
Why we are setting these state's variables here and not in the first callback? We are waiting for this.props.cotfAllowedLocations
to change or somehow influence rendering order? I don't understand idea behind it.
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.
It seems to me that we want to reset something? Why would we need that?
hasChildrenClassName, | ||
isLoadingChildrenClassName, | ||
isNotSelectableClassName, | ||
].join(' '), |
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.
👍
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.
I would go for helper function accepting object { 'some-class-name': true | false }
.
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.
I get your point, but we won't do it now.
@sunpietro looks like it needs a rebase. :) |
892de3a
to
e0bade3
Compare
@micszo branch has just been rebased |
Follow-up issue https://jira.ez.no/browse/EZP-29759. |
@sunpietro you can merge it up |
https://jira.ez.no/browse/EZP-29746