appbaseio / reactivesearch

Search UI components for React and Vue
https://opensource.appbase.io/reactivesearch
Apache License 2.0
4.89k stars 466 forks source link

ReactiveList renders "undefined" as CSS class #416

Closed thebuilder closed 6 years ago

thebuilder commented 6 years ago

Issue Type: Bug

Platform: Web

Description: The wrapping <div> rendered by ReactiveList contains the CSS class undefined.

<div class="undefined">...</div>

Screenshots: Screenshot

Minimal reproduction of the problem with instructions: https://l57vrrjvyl.codesandbox.io/

Reactivesearch version: 2.6.8

Browser: all

vivek12345 commented 6 years ago

@thebuilder @divyanshu013 It seems the problem is here https://github.com/appbaseio/reactivesearch/blob/dev/packages/web/src/components/result/ReactiveList.js#L454

Also for the sandbox url, can I get the codesandbox code for that in order to replicate the issue on my local machine and see if this line is what is causing the issue.

divyanshu013 commented 6 years ago

@vivek12345 here's the sandbox link: https://codesandbox.io/s/l57vrrjvyl

It looks like you've found the root cause. You could set the innerClass prop as empty string by default ''. That should do the trick.

vivek12345 commented 6 years ago

@divyanshu013 Alright will do that and test the same.I was also thinking as to why this sandbox code did not get the listClass.From what I could see it comes from styles/card.js which is a simple css in js container style. Do you think that's also a matter of concern and should be checked?

divyanshu013 commented 6 years ago

listClass prop is passed by ResultList and ResultCard which internally render a ReactiveList. So, when using ReactiveList by itself, it's getting undefined. You may check the className after applying the fix. I think the problem should be resolved with it.

vivek12345 commented 6 years ago

@divyanshu013 How do I test it now? I forked your repo, then did a npm link to this locally installed clone of the repo, but I am still not sure as to how do I test my changes locally before sending a PR. Can you guide here?

vivek12345 commented 6 years ago

@divyanshu013 Don't bother about the previous comment, I have figured it out. The problem with keeping innerClass as empty string is that it fails the prop types check.

Prop types expect innerClass to be an object of type types.style.

vivek12345 commented 6 years ago

@divyanshu013 It seems the solution here is making listClass default to empty string and not the innerClass

metagrover commented 6 years ago

We will roll this out in the next release 🎉

Thank you for the contribution guys!

divyanshu013 commented 6 years ago

My bad, I intended to type listClass 😅, thanks for contributing 🤝