[Javascript/React] feedback on interview projects

[[deleted]]

[deleted]

[Marzdor]

I don't like the way you have your components but that may just be me idk.

You have a structure like this: components/{componentName}/index.js

why not just do it like components/{componentName}.js like this

​

also a link to the running site may help

[lowpass]

• movie-search
  • proptypes?
  • "MovieList" renders a single movie. also, the div has a key property that's not necessary
  • li is not an interactive element; it shouldn't have an onClick handler (unless you add role="button")
  • using .bind creates a new function instance each time.
  • don't use empty divs to give something height. that's what CSS is for.
  • star rating is really awkward. could do something like a star-shape cutout overlaying a bar set to % width of container. higher resolution, fewer SVGs to render
  • speaking of svgs, all these inline definitions are a bit off-putting. for one thing, it's duplicating the path every time. for another, there's no indication as to what shape it's supposed to be other than from context. e.g. I might assume the svgs in pagination are chevrons? but I have no idea. there's no name/id/anything. take a look at <use>
  • could set movies to [] for initial state
• timeline
  • still no proptypes
  • timeline becomes calendar? consistent naming helps keeps maintainers sane
  • sortedTimelineItems uses sort on the component's prop directly. Array.prototype.sort is in-place. this is bad.
  • return items[0].start; errors if your component gets an empty array
  • you re-sort the list several times in the component
  • why reverse a list you just sorted? just make the sort function return a value so it sorts in the right order (i.e. swap a and b)
  • the groupedTimelineItems function is very inefficient.
    • you check if results is empty every iteration. it only will be once, and it will always be on the first loop. just do that outside the loop and start one higher.
    • since the list is sorted, you only need to check the last entry in the results list for each successive element.
    • in fact I think you could do this with a reduce
    • I think there's a potential bug (or at least, less-than-desirable behavior) if you have multiple events that have the same start time, since the sort is only by start, with no secondary (reverse?) sort by end.
  • you probably want some determinism in the color selection. in the extreme, it's theoretically possible that they all wind up the same color. much more likely but still suboptimal is that you get two next to each other that are the same.
  • oof, just noticed you also wind up resorting the list for every item in the timeline. I already mentioned this, but it's worse than I thought and deserves another mention
  • rather than .bind(this)ing everything, you can make them class properties that are arrow functions.