Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NUKE tab for dotnet tools #8578

Merged
merged 10 commits into from
May 14, 2021
Merged

Add NUKE tab for dotnet tools #8578

merged 10 commits into from
May 14, 2021

Conversation

matkoch
Copy link
Contributor

@matkoch matkoch commented May 10, 2021

This implements NuGet/Home#10782

zhhyu and others added 9 commits March 22, 2021 17:32
[ReleasePrep][2020.03.09] RI of dev into master
…8464)

* reject empty readme file & fix error message for invalid extension
[ReleasePrep][2021.04.08]RI dev into main
[ReleasePrep][2021.04.15]RI dev into main
* change sql get vulunerabilities by package id

* Adjusted logic to provide correct groupings, adjusted UTs

Co-authored-by: Drew Gillies <drewgil@microsoft.com>
[ReleasePrep][2021.04.26] RI of dev into main
@matkoch matkoch requested a review from a team as a code owner May 10, 2021 17:49
@joelverhagen
Copy link
Member

Hey @matkoch, could you provide an example screenshot to help people assess the look n' feel?

Could you try a long package ID and version on a narrow window size/phone view port?

Looks good overall!

@matkoch
Copy link
Contributor Author

matkoch commented May 10, 2021

@joelverhagen example was included in the original PR (NuGet/Home#10782 (comment)). only thing I've changed is to have a --version in between to align with other commands:

image

I'm not sure what exactly I should test there. I basically did simple pattern matching according to the existing tabs. Is there anything I have overlooked?

@joelverhagen
Copy link
Member

Regarding testing, what I've done for past tabs is make a package locally with a very long package ID and version then push it to my local running NuGetGallery. Then, I open the package details page and resize the window to various widths to make sure the text wrapping looks okay for the command. For example, this package ID causes wrapping: https://www.nuget.org/packages/GovUk.Education.SearchAndCompare.Ui.Shared/0.5.4-dependabot-npm-and-yarn-src-webpack-4-34-0.34

Regarding the screenshot, it helps the devs and PMs on my team get an accurate preview of what the tab is going to look like prior to approving the PR. Could you update the screenshot to include the --version part by running the code locally and capturing a screenshot?

Another thing I noticed is the yellow "contact maintainers" banner is linking to more of a docs page than a "get help" or "support" page. For example, Cake and Paket have dedicate support pages so that users can more easily get help if they need it:

What do you think about getting a similar page for NUKE?

@matkoch
Copy link
Contributor Author

matkoch commented May 11, 2021

image

image

image

@matkoch
Copy link
Contributor Author

matkoch commented May 11, 2021

@matkoch
Copy link
Contributor Author

matkoch commented May 11, 2021

@joelverhagen I changed it to nuke.build now. I think this provides all the necessary links (docs, GitHub, Slack, Twitter), and also gives a good introduction for those noticing it for the first time.

@joelverhagen
Copy link
Member

Thanks for the screenshots!

I think link # 2 is the best (https://github.com/nuke-build/nuke/issues/new/choose). This seems to have Slack and GitHub issues. @JonDouglas - thoughts on the best link for the "Please contact its maintainers for support." link in the screenshots above?

@matkoch
Copy link
Contributor Author

matkoch commented May 11, 2021

@joelverhagen just to explain my reasoning here: I'd like to help people helping themselves. That's why my first suggestion was the docs page that explains how CLI tools are used, which is exactly the point of the command. In any case, I could add another link to the docs also for the "new issue" link.

Edit: anyways, updated the issue links and changed the one for the tab.

@joelverhagen joelverhagen self-assigned this May 13, 2021
@joelverhagen joelverhagen changed the base branch from main to dev May 13, 2021 22:43
@joelverhagen
Copy link
Member

FYI I changed the base branch to dev. The diff looks fine so I don't think you'll need to change anything. But GitHub has merge UI has surprised me before 😄.

The commit list above could be eliminated with by rebasing your commit onto dev branch
image

@joelverhagen joelverhagen merged commit 066ab58 into NuGet:dev May 14, 2021
@matkoch
Copy link
Contributor Author

matkoch commented May 14, 2021

Thank you @joelverhagen !

@joelverhagen
Copy link
Member

You're most welcome 😃. Thanks for pushing through the design and implementation!

@matkoch
Copy link
Contributor Author

matkoch commented May 14, 2021

Next step I'd like to approach is NuGet/Home#10833

Out of curiosity, how likely would it be for this to get implemented? I can help, but I'm not sure if it aligns with plans and architecture.

@joelverhagen
Copy link
Member

@matkoch, given this change is on the client side, I'll leave the discussion on that issue. I think there may be a blocker on another bug so we'll have to untangle the requirements and blockers carefully!

@lyndaidaii lyndaidaii mentioned this pull request May 18, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants