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

9426 migrate workflow pages to command menu #9515

Merged
merged 19 commits into from
Jan 13, 2025

Conversation

bosiraphael
Copy link
Contributor

@bosiraphael bosiraphael commented Jan 9, 2025

Closes twentyhq/core-team-issues#53

  • Removes command menu top bar text input when the user is not on root page
  • Fixes bug when resetting command menu context
  • Added animations on command menu open and close
  • Refactored workflow visualizer code to remove unnecessary rerenders and props drilling
Enregistrement.de.l.ecran.2025-01-10.a.11.01.28.mov

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 PR migrates workflow-related pages to the command menu system, adding support for workflow step management and improving context reset functionality.

  • Added 4 new workflow pages in CommandMenuPages.ts: trigger type selection, action selection, step viewing, and step editing
  • Updated CommandMenuTopBar.tsx to conditionally render search input only on root page
  • Added workflow components to COMMAND_MENU_PAGES_CONFIG map with corresponding right drawer mappings
  • Implemented resetCommandMenuContext function in useCommandMenu hook to properly reset targeted records, selected records count, and view type
  • Enhanced hotkey handling in useCommandMenuHotKeys.ts to only reset context on Backspace/Delete when on root page

6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@Devessier Devessier self-assigned this Jan 9, 2025
@Devessier Devessier self-requested a review January 9, 2025 16:42
@Devessier
Copy link
Contributor

Some regressions we can fix in a follow-up PR that someone else could be responsible of:

CleanShot.2025-01-09.at.17.45.29.mp4

@bosiraphael
Copy link
Contributor Author

Good catches @Devessier ! I will try to fix these regressions in this PR :)

@Devessier
Copy link
Contributor

I think a container should take all the available height.

CleanShot 2025-01-09 at 17 54 49@2x

CleanShot.2025-01-09.at.17.55.49.mp4

@Devessier
Copy link
Contributor

I spotted this code:

const StyledCommandMenu = styled.div`
  background: ${({ theme }) => theme.background.secondary};
  border-left: 1px solid ${({ theme }) => theme.border.color.medium};
  box-shadow: ${({ theme }) => theme.boxShadow.strong};
  font-family: ${({ theme }) => theme.font.family};
  height: 100%;
  overflow: hidden;
  padding: 0;
  position: fixed;
  right: 0%;
  top: 0%;
  width: ${() => (useIsMobile() ? '100%' : '500px')};
  z-index: 30;
`;

I don't know where this code comes from but I find it strange to call a hook in a style expression. I don't know how emotion works internally and can't be sure it won't break the Rules of Hooks. It would be more idiomatic to let the StyledCommandMenu component take an isMobile property, which would be declared in the parent component like so:

const isMobile = useIsMobile()

<StyledCommandMenu isMobile={isMobile} />

I spotted this code at several places. Not sure about them.

CleanShot 2025-01-09 at 18 02 02@2x

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

It looks good to me! As you said, you want to fix regressions in this PR. I will approve it once you've done them 😉 Thank you!

@bosiraphael
Copy link
Contributor Author

I spotted this code:

const StyledCommandMenu = styled.div`
  background: ${({ theme }) => theme.background.secondary};
  border-left: 1px solid ${({ theme }) => theme.border.color.medium};
  box-shadow: ${({ theme }) => theme.boxShadow.strong};
  font-family: ${({ theme }) => theme.font.family};
  height: 100%;
  overflow: hidden;
  padding: 0;
  position: fixed;
  right: 0%;
  top: 0%;
  width: ${() => (useIsMobile() ? '100%' : '500px')};
  z-index: 30;
`;

I don't know where this code comes from but I find it strange to call a hook in a style expression. I don't know how emotion works internally and can't be sure it won't break the Rules of Hooks. It would be more idiomatic to let the StyledCommandMenu component take an isMobile property, which would be declared in the parent component like so:

const isMobile = useIsMobile()

<StyledCommandMenu isMobile={isMobile} />

I spotted this code at several places. Not sure about them.

CleanShot 2025-01-09 at 18 02 02@2x

Yes indeed we shouldn't do that, I copy pasted this component from another file and let that slide. good catch

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

I spotted another small bug.

Copy link
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Thanks for your hard work! It works really great and the code is clean. Your refactors in the workflows look good.

@bosiraphael bosiraphael merged commit 530a185 into main Jan 13, 2025
34 checks passed
@bosiraphael bosiraphael deleted the 9426-migrate-workflow-pages-to-command-menu branch January 13, 2025 15:53
prastoin pushed a commit that referenced this pull request Jan 14, 2025
Closes twentyhq/core-team-issues#53 

- Removes command menu top bar text input when the user is not on root
page
- Fixes bug when resetting command menu context
- Added animations on command menu open and close
- Refactored workflow visualizer code to remove unnecessary rerenders
and props drilling


https://github.com/user-attachments/assets/1da3adb8-220b-407b-9279-30354d3100d3
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.

Migrate workflow pages to command menu
3 participants