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

feat(PageItem): use "as" property as an anchor element #6754

Merged
merged 1 commit into from Jan 17, 2024

Conversation

dilame
Copy link
Contributor

@dilame dilame commented Jan 12, 2024

This Pull Request addresses the issue of limited routing flexibility in the PageItem component (#6753). By introducing the anchorAs prop, it allows the specification of a custom component for rendering links, thereby enabling seamless integration with custom routing libraries like Next.js's <Link>.

Changes Made:

  • Added a new prop anchorAs to PageItemProps.
  • Updated the PageItem component to use anchorAs as the component type for rendering the link.
  • Ensured backward compatibility by defaulting anchorAs to the existing Anchor component from @restart/ui/Anchor.

Benefits:

  • Enhances the flexibility and usability of PageItem in applications with custom routing requirements.
  • Allows integration with Next.js's Link component or any other custom component for link rendering.
  • Maintains backward compatibility, ensuring that existing implementations are unaffected.

I believe this enhancement will significantly benefit developers working with custom routing solutions and look forward to feedback and suggestions.

@kyletsang kyletsang linked an issue Jan 15, 2024 that may be closed by this pull request
@kyletsang
Copy link
Member

Could you change anchorAs to as? Should be fine to use that as the link is the main component in here anyway.

@dilame
Copy link
Contributor Author

dilame commented Jan 17, 2024

@kyletsang there seem to be a conflict with interface BsPrefixRefForwardingComponent, because it accepts interface BsPrefixProps extends AsProp in constructor, so if i just rename anchorAs to as i receive error

TS2322: Type ({ children, ...props }: any) => Element is not assignable to type ("li" & ElementType<any>) | undefined
Type ({ children, ...props }: any) => Element is not assignable to type "li" & FunctionComponent<any>
Type ({ children, ...props }: any) => Element is not assignable to type "li"
helpers.ts(17, 3): The expected type comes from property as which is declared here on type
IntrinsicAttributes & Omit<Omit<DetailedHTMLProps<LiHTMLAttributes<HTMLLIElement>, HTMLLIElement>, "ref"> & { ...; }, BsPrefixProps<...> & PageItemProps> & BsPrefixProps<...> & PageItemProps & { ...; }

@kyletsang
Copy link
Member

Try removing the anchorAs definition entirely in PageItemProps. The as prop is already inherited from the base interface.

@dilame
Copy link
Contributor Author

dilame commented Jan 17, 2024

That's exactly what i did. If you look at the error message from TypeScript, you will see that there is a conflict with the base interface.
The error is produced by this code from test

<PageItem
          as={({ children, ...props }) => (
            <a {...props} data-anchor="custom">
              {children}
            </a>
          )}
        />

@kyletsang
Copy link
Member

Try the following:

it('should support custom anchor element', () => {
  const Component = ({ children, ...props }) => (
    <a {...props} data-anchor="custom">
      {children}
    </a>
  );
  const { container } = render(<PageItem as={Component} />);
  const pageItemElem = container.firstElementChild!;
  const pageItemInnerElem = pageItemElem.firstElementChild!;
  pageItemInnerElem.getAttribute('data-anchor')!.should.equal('custom');
});

@dilame dilame changed the title feat(PageItem): implement "anchorAs" property feat(PageItem): use "as" property as anchor an element Jan 17, 2024
@dilame dilame changed the title feat(PageItem): use "as" property as anchor an element feat(PageItem): use "as" property as an anchor element Jan 17, 2024
@dilame
Copy link
Contributor Author

dilame commented Jan 17, 2024

Thanks, it works.

I was not able to change commit message, should i create a new PR, or you will manage it with squash?

@kyletsang
Copy link
Member

Yeah that's fine, I'll squash it. Thanks!

@dilame
Copy link
Contributor Author

dilame commented Jan 17, 2024

I also kindly ask you to release a new version to NPM, it's already my second PR that i'm waiting to use in my project:)

@kyletsang kyletsang merged commit 430b0c9 into react-bootstrap:master Jan 17, 2024
10 of 12 checks passed
@kyletsang
Copy link
Member

I just published v2.10.0.

Thanks for the contributions!

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.

Feature Request: Custom Anchor element for PageItem
2 participants