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

fix: allow column groups create form only for map_nodes [YTFRONT-3901] #168

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

shishovKA
Copy link
Contributor

@shishovKA shishovKA commented Oct 20, 2023

@shishovKA shishovKA force-pushed the column-groups-map-nodes-only branch 2 times, most recently from 431b69d to 0005c56 Compare October 20, 2023 17:11
@shishovKA shishovKA marked this pull request as ready for review October 20, 2023 18:25
@shishovKA shishovKA marked this pull request as draft October 23, 2023 08:43
@shishovKA shishovKA marked this pull request as ready for review October 23, 2023 09:27
const props = {
path,
loadAclDataFn: () => loadAclData({path, idmKind}),
columnGroups,
cluster,
hasCreateForm: allowedNodeTypes.includes(nodeType),
Copy link
Collaborator

Choose a reason for hiding this comment

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

At the moment allowedNodeTypes looks like an unnecessary variable:
nodeType === 'map_node'

path,
loadAclDataFn,
cluster,
hasCreateForm = false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like editing is still allowed for all node types.
Will be an edited item applied only for current path?
Do we need to allow edit only for map_nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by now, we are expecting to have column groups only for 'map_nodes'

i've asked the team whether should i remove the whole Column groups component for other node types, but they asked not to do it right now. They have some plans for it (maybe in some future, they will provide smth like @effective_column_groups attribute to display it for tables and other child nodes)

So i think, we are safe not to worry about editing, because user will be not allowed to create column groups for that nodes -> so the table will be empty

Copy link
Contributor Author

@shishovKA shishovKA Oct 23, 2023

Choose a reason for hiding this comment

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

I changed my mind)

I am going to hide actions column (delete and edit) for other node types (except map_node)
Also will rename hasCreateForm prop to readOnly or smth more suitable to handle both: hiding Add+ button and action buttons

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer allowEdit instead of readOnly cause it by default it is disabled (but readOnly is good too).

@shishovKA shishovKA force-pushed the column-groups-map-nodes-only branch from 0005c56 to 773dcda Compare October 23, 2023 16:23
@shishovKA shishovKA merged commit 32a8bf0 into main Oct 23, 2023
7 checks passed
@shishovKA shishovKA deleted the column-groups-map-nodes-only branch October 23, 2023 20:00
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