fix(sidebaritem): fix sidebar item with next link #438

myabeaver · 2022-11-24T17:28:44Z

Description

This PR fixes the next/link component for the SidebarItem.

Fixes #430

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules
I have checked my code and corrected any misspellings

codecov · 2022-11-24T17:34:48Z

Codecov Report

Base: 97.41% // Head: 97.42% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (bb26a5d) compared to base (4934a89).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #438   +/-   ##
=======================================
  Coverage   97.41%   97.42%           
=======================================
  Files         127      127           
  Lines        5460     5471   +11     
  Branches      443      443           
=======================================
+ Hits         5319     5330   +11     
  Misses        141      141

Impacted Files	Coverage Δ
src/lib/components/Sidebar/SidebarItem.tsx	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rluders · 2022-11-26T13:20:39Z

I'm not sure about this one... maybe @tulup-conner could give it a look.

tulup-conner · 2022-11-27T09:10:57Z

I don't totally understand what this does TBH. I don't really use Next. I don't think it makes sense for the library to have Next-specific patches.

rluders · 2022-11-27T10:04:44Z

Yes. I agree with you one this one @tulup-conner, I don't want to see specific-nextjs or any specific stuff landing at the flowbite-react. I have the same feeling that this is too specific. I'm wondering if #430 can't be solved in some more generic approach.

rluders · 2022-11-27T10:05:46Z

Also, I'm seeing the "Legacy" word there... So, is this for new Next.js version or just to support oldest versions? I mean, if this is a legacy support, I'll say: we don't care.

src/lib/components/Footer/FooterLink.tsx

src/lib/components/Navbar/NavbarLink.tsx

rluders · 2022-11-27T10:08:52Z

src/lib/components/Next/NextLinkLegacyBehavior.tsx

@@ -0,0 +1,15 @@
+import { useCallback, useState, type FC, type ReactElement } from 'react';


This component seems to be like a "documented solution" not a flowbite-react component. So, maybe we could add it to the documentation insted? Some "Known Issues" section?

This component is only to test the behavior. But testing it is important, because the anti-pattern (component in component) is not obvious to new React devs.

rluders · 2022-11-27T10:10:27Z

src/lib/components/Sidebar/SidebarItem.tsx

    return (
-      <ListItem>
+      <ListItem id={id} isCollapsed={isCollapsed} contentChildren={children}>


I felt like having the same id there and at line 144 will be a problem. I mean, the id should be always unique. Right? Maybe adding some suffix to the one at 144 line, something like ${id}-children.

id != id. This will be passed down to Children not printed as html. But i also feel like it's complicated to follow the code. I haven't changed the code, just moved it out of the component.

I changed:

const ComponentA: FC = (props) => { const ComponentB: FC = () => { // ... }; // ... };

to

const ComponentB: FC = () => { // ... }; const ComponentA: FC = (props) => { // ... };

But yes, now it looks weird because of id and contentChildren etc.. But i guess, let's not make to much changes and concentrate us on fixing the bug first. 😄

myabeaver · 2022-11-27T20:41:51Z

Also, I'm seeing the "Legacy" word there... So, is this for new Next.js version or just to support oldest versions? I mean, if this is a legacy support, I'll say: we don't care.

@rluders @tulup-conner We don't need it to be named NextLinkLegacyBehavior, but currently it's happening to next's Link component. The reason i added this test, was to ensure, no one, creates components inside components, like this:

const ComponentA: FC = (props) => {
  const ComponentB: FC = () => {
    // ...
  };
   // ...
};

Because it's an anti-pattern, but it's not that obvious. Currently it causes this infinite rerenderings with the Next Link button. But it can also happen to other frameworks like Gatsby if they use same approaches.

I see, that NextLinkLegacyBehavior is not the best naming. This can be renamed (UnstableReferenceTest ?), but my concerns are, that without testing it, someone will add this same component-in-component pattern again.

My suggestion would be something like this:

import { useState, useCallback, type FC } from 'react';

export interface UnstableReferenceTestProps {
	maxUpdateDepth?: number;
	children: (setRef: (ref: unknown) => void) => JSX.Element;
}

export const UnstableReferenceTest: FC<UnstableReferenceTestProps> = ({
	maxUpdateDepth = 10,
	children
}) => {
	const [count, setCount] = useState(0);
	const setRef = useCallback(
		() => {
			if (count >= maxUpdateDepth) {
				throw new Error('Maximum update depth exceeded.');
			} else {
				setCount((x) => x + 1);
			}
		},
		// eslint-disable-next-line react-hooks/exhaustive-deps
		[]
	);

	return children(setRef);
};

esko22 · 2022-11-28T23:32:25Z

Been following this a bit as I am having the same issue reported in #397 using the vanilla { Link } from "react-router-dom" producing the <a> cannot appear as a descendant of <a>. using 0.3.5.

Some of this conversation is pushing my boundaries of component rendering :) but just wanted to add a note that the issue doesn't appear to be isolated to the NextJs Link component. Not sure if there are any nuances with how NextJs extends from the core React lib or if it even does at all but I've noticed that the NextJs link shows to use href in the docs where the react router specifies to

        <Sidebar.Items className="pt-8 pr-12">
          <Sidebar.ItemGroup>
                  <Sidebar.Item className="mb-2" icon={IoMdPin}>
                      <Link to="/" >
                      Map
                      </Link>
                  </Sidebar.Item>
            </Sidebar.ItemGroup>
        </Sidebar.Items>

myabeaver · 2022-12-01T13:07:29Z

@rluders @tulup-conner So how shall we proceed?

tulup-conner · 2022-12-01T19:42:20Z

Well, I've brought this up somewhere on this repo before, but the universal standard for component libraries as far as I understand is to provide a prop, generally called as, that lets you essentially "cast" any flowbite-react elements that are/can be links as any custom link component.

<Sidebar.Item className="mb-2" icon={IoMdPin} as={Link} to="/">Map</Sidebar.Item>

I tried to do this a few months ago but I'm too stupid. Or, perhaps I just need to try again. Anyway, I think this is probably the most familiar and ergonomic solution.

rluders · 2022-12-01T20:09:59Z

I'm just wondering what would happen if the Sitebar.Item became a simple div if there is no href property attached to it, and if there is a href it should become an a otherwise, it will use the children. So, in this case, could be a Link or a Button or whatever fits to the desired action.

And all that I'm saying right now could be really dumb... I'm just wondering what could happen. 🤔

tulup-conner · 2022-12-01T22:44:10Z

That makes sense to me, too. I have a theory if we could figure out how to do the as={} it might be less code/more readable, but like I said I failed before. I'll look into this.

myabeaver · 2022-12-02T11:21:45Z

That makes sense to me, too. I have a theory if we could figure out how to do the as={} it might be less code/more readable, but like I said I failed before. I'll look into this.

@tulup-conner The as approach is not that hard. It could be simply done like the icon prop on some components. Like this:

interface FlowbiteLinkProps<P = {}> {
	as: FC<P>;
	props: P & JSX.IntrinsicAttributes;
}

function FlowbiteLink<P = {}>({ as: Component, props }: FlowbiteLinkProps<P>) {
	return <Component {...props} />;
}

And then be used like this:

<FlowbiteLink as={CustomLink} props={{ href: '#' }} />

But i would prefer @rluders approach but with a children prop.

rluders · 2022-12-02T13:41:29Z

I'm not sure if I would take my suggestion seriously. I have no idea how it is going to work and if it is a good practice at all. It was just a crazy idea. 🤣

I think that also using the as={} could be interesting for Button component and so many other components that may need nome "tag mutation".

tulup-conner · 2022-12-12T08:48:37Z

Ok, I tried again and the issue I was having with Sidebar.Item is that you can't override className, which is how Remix facilitates self-aware links via NavLink. That's because we designate it ourselves in Sidebar.Item on L62. It looks like all other properties work just fine.

I'm not sure how to work around this - it might be that I will just need to patch Remix on my apps. I'm not sure if it is something that will come up with other users/use cases where they need to override className for the as={} component and can't.

rluders · 2022-12-12T09:49:35Z

Ok, I tried again and the issue I was having with Sidebar.Item is that you can't override className, which is how Remix facilitates self-aware links via NavLink. That's because we designate it ourselves in Sidebar.Item on L62. It looks like all other properties work just fine.

I'm not sure how to work around this - it might be that I will just need to patch Remix on my apps. I'm not sure if it is something that will come up with other users/use cases where they need to override className for the as={} component and can't.

I'm wondering if my crazy idea to use the Sidebar.Item as a wrapper to do any children component wouldn't solve this. We probably gonna have to change the styling and the children's component will not receive any style directly, just from its parent.

Maybe we could give this crazy idea a try? To see what happens. WDYT?

tulup-conner · 2022-12-12T10:14:34Z

Ok, I tried again and the issue I was having with Sidebar.Item is that you can't override className, which is how Remix facilitates self-aware links via NavLink. That's because we designate it ourselves in Sidebar.Item on L62. It looks like all other properties work just fine.

I'm not sure how to work around this - it might be that I will just need to patch Remix on my apps. I'm not sure if it is something that will come up with other users/use cases where they need to override className for the as={} component and can't.

The workaround looks like...

Patch to flowbite-react

-          className={classNames(
-            theme.base,
-            isActive && theme.active,
-            !isCollapsed && isInsideCollapse && theme.collapsed.insideCollapse,
-            className,
-          )}
+          className={
+            typeof className === 'string' || typeof className === 'undefined'
+              ? classNames(
+                  theme.base,
+                  isActive && theme.active,
+                  !isCollapsed && isInsideCollapse && theme.collapsed.insideCollapse,
+                  className,
+                )
+              : className
+          }

Custom component for ease of use

export const SidebarNavLink: FC<NavLinkProps & SidebarItemProps> = function ({
  children,
  ...props
}) {
  const theme = useTheme().theme.sidebar.item;

  return (
    <FlowbiteSidebar.Item
      as={NavLink}
      className={({ isActive }: { isActive: boolean }) =>
        classNames(theme.base, isActive && theme.active)
      }
      {...props}
    >
      {children}
    </FlowbiteSidebar.Item>
  );
};

Actual links

<Sidebar.NavLink to="/brand/components/tooltip">Tooltip</Sidebar.NavLink>

The end product in the final snippet here is an ideal DX from my perspective, but I think the pieces in between can be better. Overall, if this is all I have to write to accomplish this, it isn't really that bad. The custom component I made is for convenience. You can easily just write:

<Sidebar.Item 
  as={NavLink} 
  to="/brand/components/tooltip" 
  className={({ isActive }) => 
    classNames(theme.base, isActive && theme.active)
  }
>
  Tooltip
</Sidebar.NavLink>

For example.

myabeaver · 2022-12-20T13:27:16Z

@tulup-conner I appreciate your efforts, but I think we are currently doing too much effort for a small fix.

Removing the React anti-pattern and moving the components outside the rendering cycle will solve the problem with Next.js.

If the problem persists for other libraries, we should look at that separately.

myabeaver · 2022-12-20T13:48:54Z

@tulup-conner @rluders I removed most of the code. Now it only contains the relevant part. I moved Children , ListItem and TooltipContent outside of the Sidebar.Item component. This fixes the issue with Next.js.

rluders · 2022-12-22T08:54:22Z

@myasteiner Well, I guess that this now changes the PR general goal. So, maybe we could update the title and the description.

myabeaver · 2022-12-22T11:01:33Z

@myasteiner Well, I guess that this now changes the PR general goal. So, maybe we could update the title and the description.

@rluders I've updated the title and description of the PR.

rluders requested a review from tulup-conner November 26, 2022 13:20

rluders requested changes Nov 27, 2022

View reviewed changes

fix(nextlink): fix next link for link components

bb26a5d

myabeaver force-pushed the next-link-fix branch from 68dbea5 to bb26a5d Compare December 20, 2022 13:46

rluders mentioned this pull request Dec 22, 2022

Fix router to accept nested routes sumelms/frontend#136

Open

myabeaver changed the title ~~fix(nextlink): fix next link for link components~~ fix(nextlink): fix sidebar item with next link Dec 22, 2022

myabeaver changed the title ~~fix(nextlink): fix sidebar item with next link~~ fix(sidebaritem): fix sidebar item with next link Dec 22, 2022

rluders approved these changes Dec 22, 2022

View reviewed changes

rluders merged commit 32f337b into themesberg:main Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sidebaritem): fix sidebar item with next link #438

fix(sidebaritem): fix sidebar item with next link #438

myabeaver commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

rluders commented Nov 26, 2022

tulup-conner commented Nov 27, 2022

rluders commented Nov 27, 2022

rluders commented Nov 27, 2022

rluders Nov 27, 2022

myabeaver Nov 27, 2022

rluders Nov 27, 2022

myabeaver Nov 27, 2022

myabeaver commented Nov 27, 2022 •

edited

Loading

esko22 commented Nov 28, 2022

myabeaver commented Dec 1, 2022

tulup-conner commented Dec 1, 2022 •

edited

Loading

rluders commented Dec 1, 2022

tulup-conner commented Dec 1, 2022

myabeaver commented Dec 2, 2022

rluders commented Dec 2, 2022

tulup-conner commented Dec 12, 2022

rluders commented Dec 12, 2022

tulup-conner commented Dec 12, 2022 •

edited

Loading

myabeaver commented Dec 20, 2022

myabeaver commented Dec 20, 2022

rluders commented Dec 22, 2022

myabeaver commented Dec 22, 2022

		@@ -0,0 +1,15 @@
		import { useCallback, useState, type FC, type ReactElement } from 'react';

fix(sidebaritem): fix sidebar item with next link #438

fix(sidebaritem): fix sidebar item with next link #438

Conversation

myabeaver commented Nov 24, 2022 • edited Loading

Description

Type of change

Checklist:

codecov bot commented Nov 24, 2022 • edited Loading

Codecov Report

rluders commented Nov 26, 2022

tulup-conner commented Nov 27, 2022

rluders commented Nov 27, 2022

rluders commented Nov 27, 2022

rluders Nov 27, 2022

Choose a reason for hiding this comment

myabeaver Nov 27, 2022

Choose a reason for hiding this comment

rluders Nov 27, 2022

Choose a reason for hiding this comment

myabeaver Nov 27, 2022

Choose a reason for hiding this comment

myabeaver commented Nov 27, 2022 • edited Loading

esko22 commented Nov 28, 2022

myabeaver commented Dec 1, 2022

tulup-conner commented Dec 1, 2022 • edited Loading

rluders commented Dec 1, 2022

tulup-conner commented Dec 1, 2022

myabeaver commented Dec 2, 2022

rluders commented Dec 2, 2022

tulup-conner commented Dec 12, 2022

rluders commented Dec 12, 2022

tulup-conner commented Dec 12, 2022 • edited Loading

myabeaver commented Dec 20, 2022

myabeaver commented Dec 20, 2022

rluders commented Dec 22, 2022

myabeaver commented Dec 22, 2022

myabeaver commented Nov 24, 2022 •

edited

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

myabeaver commented Nov 27, 2022 •

edited

Loading

tulup-conner commented Dec 1, 2022 •

edited

Loading

tulup-conner commented Dec 12, 2022 •

edited

Loading