-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(glossary): add ability to clone glossary term(name and documentation) from term profile menu #9170
feat(glossary): add ability to clone glossary term(name and documentation) from term profile menu #9170
Conversation
…tion) from term profile menu
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.
looking real good @olgadimova ! thank you so much for this. I have a few relatively minor comments but after these I think we'll be good to merge :)
const nodeToUpdate = props.clone | ||
? getGlossaryRootToUpdate(entityType) | ||
: entityData?.urn || getGlossaryRootToUpdate(entityType); |
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.
so i think you may have unconvered an existing bug here that can be fixed and work for both the clone case as well as all regular cases. instead of always updating the root when cloning, I think this line can be:
const nodeToUpdate = selectedParentUrn || getGlossaryRootToUpdate(entityType);
because it's the parent that needs updating in the sidebar if there is a parent, otherwise we update the root
@@ -33,6 +35,7 @@ interface Props { | |||
entityType: EntityType; | |||
onClose: () => void; | |||
refetchData?: () => void; | |||
clone?: boolean; |
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.
how about a rename to isCloning
? a little more obvious that it's a boolean flag than just clone
if you ask me
@@ -197,10 +200,21 @@ function EntityDropdown(props: Props) { | |||
</MenuItem> | |||
</StyledMenuItem> | |||
)} | |||
{menuItems.has(EntityMenuItems.CLONE) && ( |
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.
how do you feel about moving this item down below the move and delete buttons? clone feels like an additional "nice to have" while move and delete feel more like required functionality.
@chriscollins3456 This PR can be closed, cause we changed origin. We can continue discussion in #9445. I also implemented suggested changes. Thanks! |
Add ability to clone Glossary Term by using "Clone" button in term's profile menu. After cloning, user is redirected to new entity's profile page.
Checklist