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

Migrate to twenty-ui - navigation/breadcrumb #7793

Merged
merged 4 commits into from
Oct 21, 2024
Merged

Migrate to twenty-ui - navigation/breadcrumb #7793

merged 4 commits into from
Oct 21, 2024

Conversation

gitstart-app[bot]
Copy link
Contributor

@gitstart-app gitstart-app bot commented Oct 17, 2024

Description

  • Move breadcrumb components to twenty-ui


    Fixes #7534

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 the Breadcrumb component from the twenty-front package to the twenty-ui library, centralizing UI components for improved maintainability.

  • Moved Breadcrumb component to packages/twenty-ui/src/navigation/breadcrumb/components/Breadcrumb.tsx
  • Updated import statements in multiple files to use Breadcrumb from 'twenty-ui'
  • Removed packages/twenty-front/src/modules/ui/navigation/bread-crumb/components/__stories__/Breadcrumb.stories.tsx
  • Added export for navigation components in packages/twenty-ui/src/index.ts
  • Updated packages/twenty-front/tsup.ui.index.tsx to export all components from 'twenty-ui'

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

`;

const StyledDivider = styled.span`
width: ${({ theme }) => theme.spacing(2)};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The divider width is fixed. Consider making it customizable through props

) : (
<StyledText title={text}>{link.children}</StyledText>
)}
{index < links.length - 1 && <StyledDivider>/</StyledDivider>}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider making the divider customizable through props

Copy link
Member

@charlesBochet charlesBochet 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 the PR @gitstart-twenty

Feedbacks:

  • I'm surprised we are not removing the Breadcrumb.tsx component
  • And why is the story not migrated?

@gitstart-twenty
Copy link
Contributor

gitstart-twenty commented Oct 18, 2024

Thanks for the PR @gitstart-twenty

Feedbacks:

  • I'm surprised we are not removing the Breadcrumb.tsx component
  • And why is the story not migrated?

Hi @charlesBochet

Thanks for thanks for the feedback.

I'm surprised we are not removing the Breadcrumb.tsx component

Apologies, we would remove the Breadcrumb.tsx shortly.

And why is the story not migrated?

This is due to the tight coupling that currently exists in the storybook's decorator (ComponentWithRouterDecorator). In which if imported here, would necessitate import of all it's dependencies as well e.g PageDecorator which eventually leaves to even more dependencies.

We started a discussion here on discord to clarify the convention to adopt in such cases, and Lucas clarified that, we can do the same for now, and let the stories in twenty-front.

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Ok, LGTM :)

@charlesBochet charlesBochet merged commit b45511c into main Oct 21, 2024
15 checks passed
@charlesBochet charlesBochet deleted the TWNTY-7534 branch October 21, 2024 19:53
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.

2 participants