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 shortcut metadata to data models & CommandMenu #7977

Merged
merged 9 commits into from
Oct 25, 2024

Conversation

florianliebig
Copy link
Contributor

Resolves #7503

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request removes hotkeys for custom object commands in the command menu, addressing the issue of conflicting shortcuts for objects with the same first letter.

  • Modified GotoHotkeysEffectsProvider.tsx to skip creating hotkeys for custom objects
  • Updated useCommandMenu.ts to set firstHotKey and secondHotKey as undefined for custom objects
  • Added a new test case in useCommandMenu.test.tsx to verify that custom objects don't receive hotkeys
  • These changes improve the command menu's object list by preventing shortcut conflicts and streamlining navigation

3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile


return (
<GoToHotkeyItemEffect
key={`go-to-hokey-item-${objectMetadataItem.id}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: 'hokey' is misspelled. Should be 'hotkey'

Suggested change
key={`go-to-hokey-item-${objectMetadataItem.id}`}
key={`go-to-hotkey-item-${objectMetadataItem.id}`}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was wrong before, should I fix it in this scope?

Copy link
Member

Choose a reason for hiding this comment

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

That'd be great thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

result.current.commandMenu.setObjectsInCommandMenu([
{
id: 'b88745ce-9021-4316-a018-8884e02d05ca',
nameSingular: 'task',
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for creating a test! Could you please pick another name for the custom object? Like Rocket or whatever you want. Because Task is a standard object so it can be misleading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@FelixMalfait
Copy link
Member

/award 150

Copy link

oss-gg bot commented Oct 23, 2024

Awarding florianliebig: 150 points 🕹️ Well done! Check out your new contribution on oss.gg/florianliebig

@FelixMalfait
Copy link
Member

I realize we still have an issue with the new standard objects we've just introduced :'(

Screenshot 2024-10-23 at 10 40 05

I didn't expect this to happened so early....

We might have to go through the hard route and introduce a shortcut field on the backend / get it from there :/

@florianliebig
Copy link
Contributor Author

@FelixMalfait jeah saw that, too... for me as a user, I would say only frequently used items should need a hotkey - not all. So hardcoding that might be most suitable

@FelixMalfait
Copy link
Member

FelixMalfait commented Oct 23, 2024

I tried to do the backend part quickly to help you :), not fully tested but it should work hopefuly! Do you want to take over and redo the frontend?

The goal is to be able to access the .shortcut property directly on the object's metadata

@florianliebig
Copy link
Contributor Author

Thanks, can take over @FelixMalfait - some questions for the UI / UX of it:

My idea was to place it here - or is that too tight? Could also create a new section at the settings.
CleanShot 2024-10-23 at 12 15 28@2x

Do we want to allow to edit the shortcuts for the non-custom objects, too? Then we would need to add another form anyways as the existing one is disabled for those.

@florianliebig florianliebig changed the title Remove GotoHotkeys for custom object commands Add shortcut metadata to data models & CommandMenu Oct 23, 2024
@FelixMalfait
Copy link
Member

Oh nice you want to display it in settings, I didn't even think we'd do that.

cc @Bonapara what do you think?

@florianliebig
Copy link
Contributor Author

@FelixMalfait Also having a hard time getting the query ObjectMetadataItems in generated gql.ts updated. Field is now missing in the response, even though I added it in many places and ran nx graphql:generate twenty-front
CleanShot 2024-10-23 at 13 54 08@2x

@FelixMalfait
Copy link
Member

FelixMalfait commented Oct 23, 2024

@florianliebig I might have forgotten something in the backend then! you can check in the graphql console: https://twenty.com/developers/graphql/metadata ; I'll check this afternoon and let you know

Did you update the FIND_MANY_OBJECT_METADATA_ITEMS query?

@Bonapara
Copy link
Member

@florianliebig thanks for your PR! It's a great fix.

For the front-end, I think placing them in the settings is a great idea, but we should wait a bit before doing that: I would love to introduce a shortcut tab in the settings and need to consider its behavior. So let's fix the backend first, and I'll create an issue for the front-end soon!

Thanks a lot for contributing ;)

@florianliebig
Copy link
Contributor Author

Thanks for the support @FelixMalfait - I can finish it up with some tests if you like

@martmull martmull merged commit bf2ba25 into twentyhq:main Oct 25, 2024
16 checks passed
@FelixMalfait
Copy link
Member

Thanks a lot @florianliebig great work! feel free to raise a PR on adding a test or on any other issue, happy to support you if you're stuck :)

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.

Improve command menu object list
4 participants