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

IBX-1455: Redesign Content Tree for OSS #59

Merged
merged 16 commits into from
Nov 26, 2021
Merged

Conversation

GrabowskiM
Copy link
Contributor

@@ -167,22 +207,33 @@ export default class ContentTree extends Component {
);
}

isTreeCollapsed = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this in that way?

@GrabowskiM GrabowskiM requested a review from dew326 November 23, 2021 11:50
const contentTreeContainer = doc.querySelector('.ez-content-tree-container');
const contentTreeWrapper = doc.querySelector('.ez-content-tree-container__wrapper');
const btn = doc.querySelector('.ibexa-btn--toggle-content-tree');
const contentTreeContainer = doc.querySelector('.ibexa-content-tree-container');
Copy link
Contributor

Choose a reason for hiding this comment

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

Question:
Maybe we can change the name of the class because for me wrapper and container are interchangeable terms for HTML node that groups other nodes or some content

<li
class="c-popup-actions__item"
key={item.id}
onClick={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just ?

Suggested change
onClick={() => {
onClick={() => setIsExpanded((prevState) => !prevState) }

Copy link
Contributor

Choose a reason for hiding this comment

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

we can do:

Suggested change
onClick={() => {
onClick={toggleExpanded}

which for me would be the clearest solution.

const itemsStyles = {};
const allOptions = [...options, ...getHeaderActions()];

console.log(allOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(allOptions);

@GrabowskiM GrabowskiM force-pushed the IBX-1455-content-tree-oss branch from 3cc551d to 67994bc Compare November 24, 2021 13:19
@@ -19,6 +20,15 @@ class ListItem extends Component {
};
}

getPrefixActions() {
const { prefixActions } = window.eZ.adminUiConfig.contentTreeWidget;
Copy link
Contributor

@dew326 dew326 Nov 24, 2021

Choose a reason for hiding this comment

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

I don't think this is the best name. Maybe secondaryItemActions, beforeItemActions, preItemActions?

@@ -167,22 +207,33 @@ export default class ContentTree extends Component {
);
}

isTreeCollapsed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isTreeCollapsed() {
checkIsTreeCollapsed() {

Comment on lines 25 to 27
const prefixActionsArr = prefixActions ? [...prefixActions] : [];

return prefixActionsArr.sort((prefixActionA, prefixActionB) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would be clearer?

Suggested change
const prefixActionsArr = prefixActions ? [...prefixActions] : [];
return prefixActionsArr.sort((prefixActionA, prefixActionB) => {
if (!prefixActions) {
return [];
}
return [...prefixActions].sort((prefixActionA, prefixActionB) => {

const toggleExpanded = () => {
setIsExpanded((prevState) => !prevState);
};
const getHeaderActions = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it identical to getPrefixActions from ListItemComponent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It takes data from different property

<li
class="c-popup-actions__item"
key={item.id}
onClick={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we can do:

Suggested change
onClick={() => {
onClick={toggleExpanded}

which for me would be the clearest solution.

@dew326 dew326 merged commit 652ad46 into main Nov 26, 2021
@dew326 dew326 deleted the IBX-1455-content-tree-oss branch November 26, 2021 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants