Initial thoughts

[chandlerprall]

In no particular order

• I like how you handled theme switching
• I'd prefer to see Chrome refactored into multiple function components
• The style prop passed to EuiHeaderSectionItem in Chrome should probably be moved to the scss file and referenced with a className, to match the existing pattern.
• The MyComponent link ("Go to page 2") throws a React warning as NextJsLink adds a ref to its children. Is there a way to programatically trigger navigations in Next similar to https://github.com/elastic/eui/blob/master/wiki/react-router.md#2-adapter-component ?

[pugnascotia]

@chandlerprall Thanks for the feedback, I think I've addressed everything. Could you take another look?

[thompsongl]

I don't have any feedback beyond what Chandler already provided.

This is fantastic

The biggest question I'd like answered is: Do you have any insight for making it easier to consume EUI in this environment? Beyond the server-side polyfills (which hopefully we can sort out in the EUI build), what was counter-intuitive or required a fix that you consider brittle?

[pugnascotia]

Well, I'd say these were the main issues:

• Including the theme files was a bit of a pain, and involved configuring CopyWebpackPlugin manually, and then I wanted hashes in the file names, so that was more code.
• Injecting the themes was fiddly. You'll notice I pass a data structure in a JSON string. 🙈
• The next/link component is little awkward since you have to add code to move your site under a subdirectory. It's not very clearly documented on the Next site, so I went searching for blog posts. There's also the think with next/link passing a ref to its child, which doesn't work for function components. Short of changing components to classes, it requires wrappers such as NextEuiButton.

So...without building something more complete with the starter, I'm not sure what else I'd want to address in EUI, besides the polyfills. The pages do render on the server, so...that's good 🎉

[chandlerprall]

Changes look good, few things on NextEuiButton - I'll start with the definition I ended up with and list the issues it fixes

type EuiButtonProps = React.ComponentProps<typeof EuiButton>
const NextEuiButton = forwardRef<HTMLAnchorElement | HTMLButtonElement, EuiButtonProps>((props, ref) => {
  return (
    // @ts-ignore EuiButton's ref is an HTMLButtonElement or an HTMLAnchorElement, depending on if `href` prop is passed
    <EuiButton {...props} buttonRef={ref}>
      {props.children}
    </EuiButton>
  )
})

ref type

The ref is forwarded down to either an HTMLButtonElement or HTMLAnchorElement, depending on if href is present. The typescript error you received comes from specifying the ref as typeof EuiButton instead. Ideally you'd be able to re-use the definition with EuiButtonProps['buttonRef'], but the way we switch the type if href is passed creates a lot of headaches. There is a Big, Complicated way to get have this component identify which element is represented by the ref but it isn't at all worth doing, so I opted for the simple union and ignored the new typescript error.

NextEuiButton type

Specifying NextEuiButton as ComponentType<EuiButtonProps> constricts the type created by forwardRef, effectively undoing the ref. When trying to pass a ref TypeScript errors with

Error:(27, 24) TS2322: Type '{ children: string; ref: MutableRefObject<HTMLButtonElement | undefined>; }' is not assignable to type 'IntrinsicAttributes & EuiButtonProps & { children?: ReactNode; }'. Property 'ref' does not exist on type 'IntrinsicAttributes & EuiButtonProps & { children?: ReactNode; }'.

Removing the explicit ComponentType<EuiButtonProps> type from NextEuiButton resolves this, as the result from forwardRef is already properly typed.

EuiButtonProps doesn't accept children

This, I assume, contributed to some of the above typings. The ComponentType<EuiButtonProps>. EuiButtonProps is not the full definition for the component's props, instead it only has CommonProps and props specific to EuiButton (e.g. no children, this is bug in EUI's exports).

[pugnascotia]

I've updated the button component as you suggested. I've also added some routing examples, including catch-all routing with a placeholder page.

[miukimiu]

Hi @pugnascotia,

It's looking great! 🎉

It would be awesome if we could also use this starter as a Codesandbox template: https://codesandbox.io/docs/templates

Just an idea! 🙂

[pugnascotia]

I've added support for the Amsterdam themes too now.

[pugnascotia]

All - I've done a fair bit more work on this, so I'd appreciate another look.

[pugnascotia]

@thompsongl since you asked about what EUI could do - maybe we could somehow stop EUI loading all of Highlight.js' themes, since otherwise in order to keep the bundle size down you have to do some Webpack hacking and manually load the languages you want. The byte saving is significant.

[pugnascotia]

@snide would love your thoughts too - I've gone a little past what the Gatsby starter does, but I'm happy to do more.

[chandlerprall]

Your updates look great! Only positive comments at this time :). Specifically I think you've found a really good middle ground for a newcomer to Next.js while showcasing EUI.

[snide]

I've run this locally and already gave @pugnascotia some feedback. I still think it'd be nice to mirror the prettier setup in EUI https://github.com/elastic/eui/blob/master/.prettierrc

Thanks for doing that with eslint.

I do have a question for how we should keep these things up to date. Usually I'm just doing small bumps to the Gatsby one. Should we point to latest?

[snide]

Also, I can set up the codesandbox template. As soon as we're good on this one (and merge the other codesanbox PR) i was going to put together a get started guide on the EUI docs that pointed to both of these.

[pugnascotia]

I've applied EUI's Prettier config to the Next starter. I meant to say, I did try using Kibana's lint plugin, but things got sticky and I removed it again. As you saw, I added a bunch of other things from EUI, e.g. accessibility and hooks.

When you say,

Should we point to latest?

do you mean the latest EUI? I guess that would mostly work, except for when it doesn't and we break something. My instinct would be to leave the starters alone, tested without whatever version they currently depend upon. We could update them on every major EUI release, which shouldn't take much time at all in the normal course of things, and would also present an opportunity to keep the rest of the starter's dependencies up-to-date.

[pugnascotia]

Any more thoughts, anyone?

[snide]

I think you're good. Next step is to link to these from our docs, which I can handle. Think you're OK and making it public, then closing out that EUI issue.

[pugnascotia]

Repo is now public.

[pugnascotia]

You can now use the Next starter on CodeSandbox - simply visit:

https://codesandbox.io/s/github/elastic/next-eui-starter

This was actually a complete faff, because CodeSandbox has a limit on the number of files in any repository that you import, and using /docs for GitHub pages meant that CodeSandbox would fail to import the repository. So now we use the gh-pages branch for the static export to GitHub, which meant changing how yarn build-docs works.

I've added links in the starter's index page and the project's README.