Pr/108

[tansongyang]

What kind of change does this PR introduce? Enhancement

What is the current behavior? See discussion in #108.

What is the new behavior? Add a modal that lets you search npm packages.

Checklist:

• Documentation
• Tests
• Ready to be merged
• Added myself to contributors table

This is definitely not done yet, but I wanted to get an initial review before moving forward.

New behavior looks like this (unstyled).

What do people think of this workflow?

[CompuIves]

Awesome!! This is great! I have an idea on how it could look like. Lemme draw something...

This is inspired a lot by how yarnpkg shows the list 😄 .

The idea is that you can navigate the list of dependencies with up/down keys. When you press enter or click one it will add the latest version, but if you click the dropdown it will open a list of versions of which you can select one.

Curious what you think!

[tansongyang]

I like that idea as well and I'm happy to do it.

However, I have a question about the sandbox. I tried to change the action of dependency-not-found-error.js to this:

const onConfirm = (name, version) => {
  dispatch(actions.source.dependencies.add(parsedName, version));
};
// In my branch, I am able to import `store`
store.dispatch(
  modalActionCreators.openModal({
    width: 600,
    Body: <SearchDependencies onConfirm={onConfirm} />,
  })
);

The goal was to make the "Add foo as dependency" button have the same workflow as the "Add Package". However, the modal was not opening. Is this expected behavior? Does the sandbox prevent redux actions from being received in the app?

[CompuIves]

Ah, yes, that won't work because that error is executed in the sandbox environment. You need to do something similar as dispatch(actions.source.dependencies.add(parsedName, version));. But this is a bit of a mess right now, so I can add this functionality as well 🙂 .

[tansongyang]

OK, then I will just worry about implementing that mockup.

[CompuIves]

Hey @tansongyang! How is this going? Can I help in any way? 😄

[tansongyang]

Hi there! It's just a little slow because I generally only have time to work on these projects on the weekends, and this is turning out to be a busy weekend for other reasons. Did you have a goal time for when you wanted to see this feature be complete?

As for help, I think the only thing I would need is the correct keys to use. I copied the current keys from the yarn repo as a temporary solution, just to get something working that I could dev against.

[CompuIves]

Absolutely no problem @tansongyang. I'm just very excited for this feature 😜 .

Take your time 😄 .

[tansongyang]

I'm excited too! Thanks for the opportunity to contribute 😄

[tansongyang]

@CompuIves I thought I should let you know: work has been heavy for me for the past few weeks, and I think it's going to stay this way for a couple more. Realistically speaking, I may not have a chance to work on this PR again until mid-November.

I don't feel that I own this code or anything like that, so if the best thing for the project is for you or someone else to take over the PR, that's totally fine by me.

Of course, if we're all willing to wait a few weeks, I can get back to it.

I'll let you decide. Thanks again!

[CompuIves]

Thanks for letting us know @tansongyang, I really appreciate that!

Absolutely no problem if you have no time, don't feel pushed to work on it if you don't have time. We're currently not in a rush to get this in, if I want to get this in quickly I'll let you know and then I'll finish the PR 🙂 .

[lbogdan]

Hey @tansongyang, you should probably try merging with the master branch, there were a lot of (big) changes since you started this.

[tansongyang]

@CompuIves Thanks! I'll take a look at using Downshift later this week.

@CompuIves @lbogdan Scoped packages seem to be an issue in the npm-search package. This happens on yarnpkg.com as well. For example, @angular/core does not appear until page 3, and it says it has 0 downloads. Perhaps the solution is to add paging controls?

[lbogdan]

Or maybe switch to https://npms.io/ (i.e. https://api.npms.io/v2/search?q=@angular/core )?

[CompuIves]

Hmm, yes. The dependency should at least be there to add, otherwise it can be very confusing. I'm not sure if this can be fixed easily, I'll ask someone from Algolia first.

[tansongyang]

Updates

• Use codesandbox keys.
• Use Downshift to get arrow navigation.
• Add paging.

[CompuIves]

Wow @tansongyang, this is awesome! Thank you very much, it's perfect.

Could you fix the lint errors and fix the jest errors? I think you can fix jest by just merging with master, had the same issue before. Then it's ready to go.

[tansongyang]

@CompuIves Oops, I forgot to do that. Hopefully, it's good now. In the future, do you think it would be valuable to add linting and testing to the precommit?

[tansongyang]

@CompuIves Is the timeout in the integration tests something I should be able to handle on my end?

[CompuIves]

This is really great, cannot wait to get this live 😄 . I'll make some very minor styling changes after merge to master (a border-bottom and text color change).

In the future, do you think it would be valuable to add linting and testing to the precommit?

Still thinking of this, I'm afraid that it will slow down my dev flow so I'd leave it off for now.

Is the timeout in the integration tests something I should be able to handle on my end?

Happens sometimes randomly, we should move the timer for that.

Again, thank you very much! This is a major feature for us 😄 😄

[tansongyang]

Great! It was fun to work on and I learned a lot. Thanks for the opportunity.

[CompuIves]

One quick question, is there a specific reason you put pagination on top?

I'm experimenting with putting it at the bottom, but maybe there are good reasons to keep it top.

[tansongyang]

I was thinking about the situation where you keep clicking "next page". If the paging controls are at the top, you are guaranteed that the button is always in the same place. So you can just keep clicking.

At first, I had the controls at the bottom. I noticed that, when I was paging, the controls would move under because each page of results would be slightly different in height. On each page, I had to readjust my mouse to get to the button.

Two solutions I can think of:

1. If we don't think the controls moving around is a problem, and it looks better at the bottom, we can just move them to the bottom.
2. If we think it's a problem but we still want controls at the bottom, we could find some way to fix the height of each page of results.

[CompuIves]

Very smart @tansongyang, we'll keep them at the top!

[CompuIves]

I helped with pushing a fix for the Algolia scoped packages indexing, let's wait for that before fully deploying. Should be today or tomorrow hopefully 😄

[Haroenv]

All scoped packages have downloads now, thanks for contributing all of you

[CompuIves]

Hey @tansongyang, one small last thing! Do you have a Twitter handle? I want to give credits where its due with the announcement 🙂 .

[tansongyang]

Sure thing! It's https://twitter.com/tansongyang. Now, I'll warn you, I haven't actually tweeted anything yet! I just used it to follow others haha.