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: fixed shortcuts population #7016

Merged
merged 20 commits into from
Oct 8, 2024
Merged

Conversation

sid0-0
Copy link
Contributor

@sid0-0 sid0-0 commented Sep 13, 2024

This PR fixes #6776

Screenshots:
image

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 addresses issue #6776 by modifying the command menu functionality to filter out deactivated objects, preventing users from accessing them through shortcuts or the command menu.

  • Added 'nameSingular' property to Command type and CommandMenuCommands for object identification
  • Updated PageChangeEffect to use objectMetadataItemsState for filtering active items
  • Modified useCommandMenu hook to include filterCommandMenu function and update setToInitialCommandMenu
  • Adjusted CommandMenu.stories to incorporate changes for handling active/inactive object metadata items
  • Implemented filtering logic to exclude deactivated objects from the command menu and shortcuts

5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

setToInitialCommandMenu,
openCreateActivity,
objectMetadataItems,
filterCommandMenu,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: filterCommandMenu is unused in this component

@FelixMalfait
Copy link
Member

Thanks a lot!
Sorry this could have been discussed upfront but tbh I think we should have done a loop and display all non isSystem +active objects like in settings, instead of filtering out from an hardcoded list. That way we could also get the icon directly from the true source in object metadata.
One thing we would lose is the shortcuts but we can bring them back later.

We have plans to isolate it into a dedicated entry point, not mixed with records:
Screenshot 2024-09-13 at 15 55 31
("Objects" would open the list of objects you can navigate to)
Maybe this would be a good first step towards that.

What do you think?

@sid0-0
Copy link
Contributor Author

sid0-0 commented Sep 15, 2024

@FelixMalfait
What exactly do you mean by isolate it into a dedicated entry point, not mixed with records (actually I don't know what records are)?

I have made changes to pick all data directly from object data.

Shortcuts are still functional as all initials are different and we won't run into any issues right now, in future we'll have to add a key for shortcut into object data itself (in case conflicts arise).

Note for reviewers: CommandMenu items' id has change from 'go-to-[labelPlural]' to uuid provided by the object. I can revert to using a generic go-to... string if required, but I didn't see any issues this would raise.

@lucasbordeau
Copy link
Contributor

Please see in GotoHotkeysEffect in order to create something dynamic. Right now it's hard coded.

@sid0-0
Copy link
Contributor Author

sid0-0 commented Sep 21, 2024

@lucasbordeau had to use a hacky way to to call react hooks dynamically in GotoHotkeyEffect.tsx.
Let me know if there is a better way to achieve this.
Verified, functioning still the same

Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

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

Used this PR to refactor a bit the app components and util into a module.

@lucasbordeau
Copy link
Contributor

@lucasbordeau had to use a hacky way to to call react hooks dynamically in GotoHotkeyEffect.tsx. Let me know if there is a better way to achieve this. Verified, functioning still the same

That's not really hacky as it's the only proper way to create a loop of hook render !

@lucasbordeau lucasbordeau enabled auto-merge (squash) October 8, 2024 15:21
@lucasbordeau lucasbordeau merged commit e662f6c into twentyhq:main Oct 8, 2024
9 of 10 checks passed
Copy link

github-actions bot commented Oct 8, 2024

Thanks @sid0-0 for your contribution!
This marks your 3rd PR on the repo. You're top 13% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
This PR fixes twentyhq#6776 

Screenshots:
<img width="1728" alt="image"
src="https://github.com/user-attachments/assets/ca061c30-ddb7-40ff-8c54-8b0d85d40864">

---------

Co-authored-by: sid0-0 <[email protected]>
Co-authored-by: Lucas Bordeau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deactivated objects can still be accessed using shortcuts & cmd+k
3 participants