monkey-tree / jammming

BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Feature Request: Summary #8

Open SidTheEngineer opened 6 years ago

SidTheEngineer commented 6 years ago

Objective/Background

Great job here 👍. You've articulated the functionality of the feature well and I think this is an excellent addition to the app. The only suggestion I had for this feature (which you covered in the caveats 👍) was the timeout mechanism so that the user doesn't get stuck with the modal open if something goes wrong. Nice work in thinking about the edge cases.

Technical Design

I think you've done an excellent job here! You've executed the functionality intuitively and it works as expected. Great work 👍

A couple of suggestions I have: Although it works appropriately, I do think things are a bit overcomplicated with the use of a portal in the situation regarding the popup modal.

... the benefit of this design is that if we want to add more pop up window in addition to ProgressBar, we could add more logic into Modal and manage the Popup window centrally and reuse Modal code.

If I'm understanding this correctly, we can actually create our modal in such a way that it will automatically render child elements within its render method, so we can do something like this:

class Modal extends React.Component {
  ...

  render() {
    return (
      <div>
        {this.props.children}
      </div>
    )
  }
}
<Modal>
  <ChildElement />
<Modal>

As an example for easier reusability in the future. Then we can simply conditionally render our Modal on some state property.

For the ProgressBar component, if connection speed is slow enough, progress will simply stall at 80%, meaning that the timer is not a correct indication of the progress of the network request that's being made to save the playlist. I think this would require much more work, so what you've got here as far as updating the user with progress UI is a great addition 👍.

Overall

Excellent work! You've not only thoroughly explained a new feature and intuitively described the steps needed to build it, you've executed it well, abiding by those steps and creating a working result. Bravo 👏. It looks like you've got a great grasp on React!

monkey-tree commented 6 years ago

Many thanks, SidTheEngineer and I'll continue to refine my work.