search

couds / react-bulma-components

React components for Bulma framework
MIT License
1.21k stars 126 forks source link

v4.0 #262

Closed kennethnym closed 3 years ago

kennethnym commented 4 years ago

This is an issue tracking this library's upgrade to v4.0.

New features:

Housekeeping:

cc @couds

edit: marked breaking changes.

kennethnym commented 3 years ago

can you open a new issue for the problem you have with gatsby and renderAs? as for the changelog i think @couds is documenting all the changes in CHANGELOG.md on the next branch.

davepwsmith commented 3 years ago

Me again (perhaps it's only me that's using this unstable branch!) - I have found that the changes in RC.3 have made gatsby fail to build: 

 ERROR #98124  WEBPACK

Generating JavaScript bundles failed

undefined

If you're trying to use a package make sure that 'undefined' is installed. If you're trying to use a local file make sure that the path is correct.

File: src/pages/index.tsx:1:94

Everything works fine with RC.2, but 3, 4 and 5 throw this.

couds commented 3 years ago

Hi @davepwsmith

I made a mistake building RC.3, check the latest one (Should be RC.5) it should work correctly

davepwsmith commented 3 years ago

Afraid not @couds - neither .3, .4 or .5 work, all with the same error.

couds commented 3 years ago

Weird. I will check this afternoon what its broken on the build and deploy a RC.6.

If possible can you share a simple repo to reproduce the issue?

Thanks

davepwsmith commented 3 years ago

https://github.com/davepwsmith/rbc-error-repro

couds commented 3 years ago

@davepwsmith Fixed on RC.7 Thanks for reporting.

davepwsmith commented 3 years ago

Just to confirm that this is all good at my end with a more complicated example than the one I gave you! Thanks.

bino98 commented 3 years ago

Hi.

Some typescript type definition maybe different. The problematic part is MenuListItemProps used in <Menu.List.Item>. https://github.com/couds/react-bulma-components/blob/v4.0.0-RC.7/src/components/menu/index.d.ts#L10

An type mismatch error occurs even if I use it according to the usage of bulma documents.

<Menu>
  <Menu.List>
      <Menu.List.Item>
        /* ↓ Type mismatch.. 'string[] | ReactElement<any, string | JSXElementConstructor<any>>[]' */
        hogehoge
      </Menu.List.Item>
  </Menu.List>
</Menu>

My team is using RC.7.

couds commented 3 years ago

Hi @bino98 I will take s look. Thanks!

davepwsmith commented 3 years ago

For what it's worth, unless you are specifically trying to restrict what people put inside these elements, I think you will pretty much always want ReactNode? It is the type that the react typings use for children (e.g. in PropsWithChildren<>).

It is a very broad type though, so if you did want to restrict further than that I would understand!

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/2034c45/types/react/index.d.ts#L194

bino98 commented 3 years ago

Thank you comments.

I forgot to write past comment, even if use a ReactNode in <Menu.List.Item>, this problem occurs.

for examples..

<Menu>
  <Menu.List>
      <Menu.List.Item>
        <HogeLabel>hogehoge</HogeLabel>
      </Menu.List.Item>
  </Menu.List>
</Menu>

sorry if I'm wrong

davepwsmith commented 3 years ago

Hi @bino98 - sorry my comment was directed at @couds. You are correct that the types aren't right.

@couds I can PR for this - but I also noticed that the title attribute is already typed as ReactNode... this means that you can put just about anything as the title here, extending to basically a whole other website! Should this not just be "string"?

I am struggling to even get the "next" branch set up at the moment though, npm i is complaining about package hashes and babel won't run...

couds commented 3 years ago

Hi @davepwsmith

I will take a look about the issue with the env.

About the title, I prefer to keep it as a ReactNode, there are scenarios where we could want a more complex title (and anchor or anything else)

couds commented 3 years ago

@davepwsmith I pushed the fix to the Item definition. Thanks.

I was not able to reproduce the issue installing the package.

btw I recommend to use npm ci instead of npm i when installing the dependencies (it will only use the lock file and its faster

bino98 commented 3 years ago

congrats for v4 release! :tada:

kennethnym commented 3 years ago

Thank you 😄