Skip to content
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-31571: Content tree view in UDW #1361

Merged

Conversation

GrabowskiM
Copy link
Contributor

@GrabowskiM GrabowskiM commented Apr 27, 2020

Question Answer
Tickets https://jira.ez.no/browse/EZP-31571
Bug fix? no
New feature? yes
BC breaks? yes
Tests pass? yes
Doc needed? no
License GPL-2.0

BC: changed parameters for loadSubtree function to object: 810d10b#diff-d555737827e39c2e1490479db6f96e76R31

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@GrabowskiM
Copy link
Contributor Author

Selection_026
Selection_028
Selection_029

@@ -156,6 +166,12 @@ export default class ContentTreeModule extends Component {
}

readSubtree() {
const { readSubtree } = this.props;

if (readSubtree) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check if it is a function

@@ -28,8 +28,13 @@ export const loadLocationItems = ({ siteaccess }, parentLocationId, callback, li
.catch(showErrorNotification);
};

export const loadSubtree = ({ token, siteaccess }, subtree, callback) => {
const request = new Request(`${ENDPOINT_LOAD_SUBTREE}`, {
export const loadSubtree = ({ restInfo: { token, siteaccess }, subtree, sortClause, sortOrder }, callback) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const loadSubtree = ({ restInfo: { token, siteaccess }, subtree, sortClause, sortOrder }, callback) => {
export const loadSubtree = ({ token, siteaccess, subtree, sortClause, sortOrder }, callback) => {

Do not pass restInfo object just pass the token and siteaccess this is more consistent with other places.

dispatchLoadedLocationsAction({ type: 'SET_LOCATIONS', data: locationsMap });

if (!multiple && !isNotSelectable) {
dispatchSelectedLocationsAction({ type: 'CLEAR_SELECTED_LOCATIONS' });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use REPLACE_SELECTED_LOCATIONS

loadedLocationsMap.forEach((location) => {
leafs.push({
children: [],
limit: 30,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why limit is hardcoded to 30?

const tree = [];
let leafs = tree;

loadedLocationsMap.forEach((location) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole method deserves to be refactored.

<div className="c-tree">
{contentTreeVisible && (
<ContentTreeModule
userId={14}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why user id is hardcoded to admin?

(containersOnly && !isContainer) || (allowedContentTypes && !allowedContentTypes.includes(contentTypeInfo.identifier));

setMarkedLocationId(locationId);
dispatchLoadedLocationsAction({ type: 'CUT_LOCATIONS', locationId: markedLocationId });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you CUT and in next line you do SET which replace the whole loadedLocations?

@GrabowskiM GrabowskiM requested a review from dew326 April 30, 2020 11:45
…b.module.js

Co-authored-by: Dariusz Szut <dew326@gmail.com>
@GrabowskiM GrabowskiM requested a review from dew326 May 4, 2020 07:15
Copy link

@katarzynazawada katarzynazawada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In the Bookmarks tab, the content tree area is narrowed.
  2. Missing translation for tooltip.

Screenshot 2020-05-07 at 12 58 50

@lserwatka lserwatka merged commit 1748607 into ezsystems:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants