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.

Sign up for GitHub

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

Jump to bottom

Remove content/en/docs/tasks/tools/install-minikube.md #24416

Merged
merged 1 commit into from
Oct 10, 2020
Merged

Remove content/en/docs/tasks/tools/install-minikube.md #24416

merged 1 commit into from
Oct 10, 2020

Conversation

nate-double-u
Copy link
Contributor

@nate-double-u nate-double-u commented Oct 7, 2020 •
edited
Loading

As per conversations in issue #23354:

  • Removing content/en/docs/tasks/tools/install-minikube.md page
  • Adding a 302 temporary redirect for /docs/tasks/tools/install-minikube/
  • Updating content/en/docs/tasks/tools/_index.md with the suggestion to add buttons to make these links more visual/see-able, including aria-labels

Additionally:

  • Updating minikube capitalization throughout page
  • Updating text throughout page for line length
  • Correcting back ticks use throughout page

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 7, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @nate-double-u!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Oct 7, 2020
@nate-double-u nate-double-u marked this pull request as draft October 7, 2020 20:06
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2020
@nate-double-u
Copy link
Contributor Author

/assign @celestehorgan

@netlify
Copy link

netlify bot commented Oct 7, 2020 •
edited
Loading

Deploy preview for kubernetes-io-master-staging ready!

Built with commit b65ddd1

https://deploy-preview-24416--kubernetes-io-master-staging.netlify.app

@nate-double-u nate-double-u force-pushed the 23354-update-install-minikube branch from b9570fb to 9747f90 Compare October 7, 2020 21:22
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Oct 7, 2020
@nate-double-u
Copy link
Contributor Author

https://deploy-preview-24416--kubernetes-io-master-staging.netlify.app/docs/tasks/tools/

@nate-double-u nate-double-u marked this pull request as ready for review October 7, 2020 21:27
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2020
@nate-double-u
Copy link
Contributor Author

/unassign @celestehorgan

@nate-double-u
Copy link
Contributor Author

/cc @celestehorgan

@celestehorgan
Copy link
Contributor

Deploy preview: https://deploy-preview-24416--kubernetes-io-master-staging.netlify.app/docs/tasks/tools/

This looks good to me!

For other approvers: how do we feel about the in-line HTML here? We don't have a {{< button >}} short-code available. Should we make a follow-up issue to create one?

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2020
kbhawkey
kbhawkey reviewed Oct 8, 2020
View reviewed changes
content/en/docs/tasks/tools/_index.md Outdated
@@ -14,6 +14,11 @@ cluster resources, and view logs.
See [Install and Set Up kubectl](/docs/tasks/tools/install-kubectl/) for information about how to
download and install `kubectl` and set it up for accessing your cluster.

<form action="/docs/tasks/tools/install-kubectl/">
<input type="submit" class="btn btn-primary" value="Install and Set Up kubectl" />
Copy link
Contributor

@kbhawkey kbhawkey Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nate-double-u . Some feedback (I may have misunderstood your intention):
What is the advantage of adding the "form" and "button"? The same link is right above?
Should the button provide a hint as to what is happening when you hit the button? (From the text
it seems like kubectl is getting ready to install).
I'd rather not add HTML to the page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did use a button, I'd prefer <button> to <input>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the button as a way to help folks get to the info they're looking for a little quicker (based on a suggestion in the issue), and tried to keep it in line with other button-links on the site (https://kubernetes.io/docs/home/#understand-the-basics)

I agree that the text could be clearer about what's going to happen next.

I also agree about not wanting to add HTML to the page, but I couldn't find a button shortcode.
Should I remove the buttons altogether?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The buttons are a bold UI statement. I would follow
the pattern of the other site buttons by permitting the text to be localized (add an
Aria label or not?). I'd like to see a tooltip or some hint that this button is forwarding
me to another page (not installing/starting up the tool).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the Aria labels, and have changed the wording of each of the buttons so that it is more obvious what they will do.

@kbhawkey
Copy link
Contributor

kbhawkey commented Oct 8, 2020

Hold for review
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2020
@irvifa
Copy link
Member

irvifa commented Oct 8, 2020

/cc @sftim

@k8s-ci-robot k8s-ci-robot requested a review from sftim October 8, 2020 16:12
@nate-double-u nate-double-u requested a review from kbhawkey October 8, 2020 23:10
@nate-double-u nate-double-u mentioned this pull request Oct 8, 2020
Closed
@kbhawkey
Copy link
Contributor

kbhawkey commented Oct 9, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020
@kbhawkey kbhawkey mentioned this pull request Oct 9, 2020
Closed
@nate-double-u nate-double-u force-pushed the 23354-update-install-minikube branch from b8937c4 to 6ea840a Compare October 9, 2020 19:01
@nate-double-u
Copy link
Contributor Author

For that I recommend using a 307 redirect (temporarily moved), so that we can change the target in the future if we need to.

I'm not sure Netlify supports 307 redirects (#24416 (comment)), so I've added a 302 redirect instead.

@kbhawkey
Copy link
Contributor

kbhawkey commented Oct 9, 2020

This looks like an improvement. Thanks!
/lgtm

@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 02914dbe59da2f075ad5ef604cc3398c7ab7bebb

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2020
@nate-double-u
Remove content/en/docs/tasks/tools/install-minikube.md
b65ddd1 
As per conversations in issue kubernetes#23354,
* Removing content/en/docs/tasks/tools/install-minikube.md page
* Adding a 302 temporary redirect for /docs/tasks/tools/install-minikube/
* Updating content/en/docs/tasks/tools/_index.md with the suggestion to add buttons to make these links more visual/see-able, including aria-labels

Additionally:
* Updating minikube capitalization throughout page
* Updating text throughout page for line length
* Correcting back ticks use throughout page

Signed-off-by: Nate W <[email protected]>
@nate-double-u nate-double-u force-pushed the 23354-update-install-minikube branch from 6ea840a to b65ddd1 Compare October 9, 2020 20:50
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2020
@irvifa
Copy link
Member

irvifa commented Oct 10, 2020

Thanks!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2020
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 145b4d3ea72ad2536ac9afa534f407a69cdde69e

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irvifa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2020
@k8s-ci-robot k8s-ci-robot merged commit a320a9e into kubernetes:master Oct 10, 2020
@nate-double-u
Copy link
Contributor Author

nate-double-u commented Oct 10, 2020 •
edited
Loading

Thanks for the help and feedback everyone!

@nate-double-u nate-double-u deleted the 23354-update-install-minikube branch October 10, 2020 15:25
@tengqm tengqm mentioned this pull request Oct 11, 2020
Merged
@nate-double-u nate-double-u mentioned this pull request Dec 22, 2020
Closed
6 tasks
This was referenced Feb 14, 2021
Merged
Closed
@dipesh-rawat dipesh-rawat mentioned this pull request Dec 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbhawkey kbhawkey kbhawkey left review comments

@daminisatya daminisatya Awaiting requested review from daminisatya

@onlydole onlydole Awaiting requested review from onlydole

@celestehorgan celestehorgan Awaiting requested review from celestehorgan

@sftim sftim Awaiting requested review from sftim

Assignees

@kbhawkey kbhawkey

@irvifa irvifa

@celestehorgan celestehorgan

@sftim sftim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nate-double-u @k8s-ci-robot @celestehorgan @kbhawkey @irvifa @sftim