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

Add resource for tree-menu-item #437

Merged
merged 27 commits into from
Aug 1, 2023

Conversation

aringocode
Copy link
Collaborator

@aringocode aringocode commented Jul 12, 2023

Changes

  1. add resource like a prop for tree-menu-item
  2. update Changelog
resource-treemenu-item.mp4

@aringocode aringocode self-assigned this Jul 12, 2023
@aringocode aringocode requested a review from Pe5h4 July 13, 2023 04:45
@aringocode
Copy link
Collaborator Author

@Pe5h4 Can you review pls?

Copy link
Collaborator

@Pe5h4 Pe5h4 left a comment

Choose a reason for hiding this comment

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

@aringocode Good, only minor thing. Also, has it been tested also when you dont have any resource given in the props?

)}
{label}
</li>
isDisabled && (resources?.indexOf(resource) == -1) && (resources?.indexOf("authz:superuser") == -1) ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode I believe you don need here the optional chaining syntax ?

@aringocode
Copy link
Collaborator Author

@aringocode Good, only minor thing. Also, has it been tested also when you dont have any resource given in the props?

@Pe5h4 I've tested it now. Assuming the resource comes back undefined. This condition (resources?.indexOf(resource) == -1) will still return -1, disabled items will not be displayed.

@aringocode aringocode requested a review from Pe5h4 July 13, 2023 10:15
@Pe5h4
Copy link
Collaborator

Pe5h4 commented Jul 13, 2023

@aringocode Good, only minor thing. Also, has it been tested also when you dont have any resource given in the props?

@Pe5h4 I've tested it now. Assuming the resource comes back undefined. This condition (resources?.indexOf(resource) == -1) will still return -1, disabled items will not be displayed.

Alright

Copy link
Collaborator

@Pe5h4 Pe5h4 left a comment

Choose a reason for hiding this comment

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

@aringocode PLease add this option to the documentation

@aringocode
Copy link
Collaborator Author

@aringocode PLease add this option to the documentation

@Pe5h4 What do you mean? Add information that there must be a necessary resource to see the disable item?

@Pe5h4
Copy link
Collaborator

Pe5h4 commented Jul 14, 2023

@aringocode PLease add this option to the documentation

@Pe5h4 What do you mean? Add information that there must be a necessary resource to see the disable item?

Basically creating the documentation for TreeMenu as we spoke of about :)

@@ -9,7 +9,7 @@ import SimpleTreeMenu from 'react-simple-tree-menu';
import TreeMenuItem from "./TreeMenuItem";

const TreeMenu = ({
data, searchOptions, ...props
data, searchOptions, hasSearch, ...props
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took hasSearch parameter out of the props because it's only used in this file

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Not sure about this action, hasSearch is a part of the TreeMenuItem and it can be eventually present in the Treemenu. Have you tested it with the hasSearch={true} option?

@aringocode aringocode requested a review from Pe5h4 July 17, 2023 06:09
@@ -9,7 +9,7 @@ import SimpleTreeMenu from 'react-simple-tree-menu';
import TreeMenuItem from "./TreeMenuItem";

const TreeMenu = ({
data, searchOptions, ...props
data, searchOptions, hasSearch, ...props
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Not sure about this action, hasSearch is a part of the TreeMenuItem and it can be eventually present in the Treemenu. Have you tested it with the hasSearch={true} option?

doc/tree-menu.md Outdated

- `data`: object or array, data in a special structure. The detailed data structure is described in the library documentation react-simple-tree-menu.

- `hasSearch`: boolean, changes the styles for the search input and affects the display of placeholder
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Its not a placeholer, it should render the Search for the TreeMenu

doc/tree-menu.md Outdated

- `hasSearch`: boolean, changes the styles for the search input and affects the display of placeholder

- `hasNodes`: boolean, if a `TreeNode` is the last node of its branch
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode I dont understand this :(

doc/tree-menu.md Show resolved Hide resolved
doc/tree-menu.md Outdated
- `resource`: string, resource granting display disabled tree-menu-item elements


#### Optional:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are more optional props, I would just add reference to this: https://github.com/iannbing/react-simple-tree-menu#api

doc/tree-menu.md Outdated
Comment on lines 30 to 43
## Props:
More parameters can be found [here](https://github.com/iannbing/react-simple-tree-menu#api).

#### Required:

- `data`: object or array, data in a special structure. The detailed data structure is described in the library documentation react-simple-tree-menu.

- `resource`: string, resource granting display disabled tree-menu-item elements


#### Optional:

- `searchOptions`: object, conducts placeholder and the dropdown component

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you can leave the very minimum of parameters in this documentation, because the parameters for a library are described in its documentation. And here will be only those parameters that we use in our component (resource, searchOption)

@aringocode aringocode requested a review from Pe5h4 July 18, 2023 06:13
Copy link
Collaborator

@Pe5h4 Pe5h4 left a comment

Choose a reason for hiding this comment

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

@aringocode Only minor refactorisation request with the resource/resources being an optional prop

}) => {
const sessionExpired = useSelector(state => state.auth?.sessionExpired);
const resources = useSelector(state => state.auth?.resources);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode I think resources should be also passed to <TreeMenu /> as a prop, like we said - resource/resources should be a optional argument. Take a look to the ButtonWithAuthz, it has a similar approach - and why I think its better to do it this way?Because we will loose a dependency to some parts of the application when we will be decoupling the UI's to microfrontend apps

)}
{label}
</li>
isDisabled && (resources.indexOf(resource) == -1) && (resources.indexOf("authz:superuser") == -1) ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, good. Test that also in Configuration part of the UI please :)

@aringocode
Copy link
Collaborator Author

@Pe5h4 I've tested it in all cases. configuration and library are displayed correctly

@aringocode aringocode requested a review from Pe5h4 July 24, 2023 07:12
@@ -9,7 +9,7 @@ import SimpleTreeMenu from 'react-simple-tree-menu';
import TreeMenuItem from "./TreeMenuItem";

const TreeMenu = ({
data, searchOptions, ...props
data, searchOptions, resources, ...props
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Not necessary, resources are part of ...props

@@ -45,6 +45,7 @@ const TreeMenu = ({
{items.map(({ reset, ...props }) => (
<TreeMenuItem
active="false"
resources={resources}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Not necessary here

@@ -24,28 +26,32 @@ const TreeMenuItem = ({
props.onClick && props.onClick();
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Remove this empty line please

const paddingLeft = 1.25 * level + 0.5;
const selected = focused ? " selected" : "";
const disabled = (isDisabled || sessionExpired) ? " disabled" : "";
// const showDisabledItems = isDisabled && resources && resource &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Remove commented obsolete code

}) => {
const sessionExpired = useSelector(state => state.auth?.sessionExpired);
// const resources = useSelector(state => state.auth?.resources);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Remove commented obsolete code please :)

...

<TreeMenu
resource={resource}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode If there is a resource in the example, there should be also resources, innit? :)


- `searchOptions`: object, conducts placeholder and the dropdown component

- `resource`: string, resource granting display disabled tree-menu-item elements
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Add also info about that resources are optional argument

@aringocode aringocode requested a review from Pe5h4 July 25, 2023 05:46
doc/tree-menu.md Outdated

- `resource`: string, resource granting display disabled tree-menu-item elements

- `resources`: array, list of resources that the user has
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode resources: array, list of resources that the user has assigned

src/components/TreeMenu/index.js Show resolved Hide resolved
<TreeMenuItem
active="false"
{...params}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Anyway, are the ...params used anywhere? I dont see the being used in the TreeMenuItem so I suppose you want to pass the {...params} to the <li> or somewhere, not to loose TreeMenuItem behavior

Copy link
Collaborator

Choose a reason for hiding this comment

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

What was originally passed to the TreeMenuItem when using just {...props} ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Well, anyway, I just wanted some information, nevertheless, your previous solution with passing {...props} together with {...params} was ok. What I wanted was just answer to my questions, why did you choose this solution and if it is propely handled in the <TreeMenuItem /> component :)

@aringocode aringocode requested a review from Pe5h4 July 28, 2023 05:44
<TreeMenuItem
active="false"
{...params}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aringocode Well, anyway, I just wanted some information, nevertheless, your previous solution with passing {...props} together with {...params} was ok. What I wanted was just answer to my questions, why did you choose this solution and if it is propely handled in the <TreeMenuItem /> component :)

@aringocode
Copy link
Collaborator Author

@Pe5h4 Yes, it is processed correctly. Props from the parent component did not come before, so I think we can leave this solution to avoid passing unnecessary parameters:

resource={props.resource}
resources={props.resources}

@aringocode aringocode requested a review from Pe5h4 July 31, 2023 03:52
@aringocode aringocode marked this pull request as ready for review August 1, 2023 05:04
@aringocode aringocode merged commit 7802fff into master Aug 1, 2023
@aringocode aringocode deleted the refactor/add-resource-for-tree-item branch August 1, 2023 05:04
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.

2 participants