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

chore(document/toolbar): replace "Quick-Edit" by "Edit on GitHub" #6065

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Apr 25, 2022

Summary

Problem

We have a "Quick-edit" feature that (presumably) nobody uses (see #4866), because the GitHub editor is much better and allows opening a PR.

Solution

  • Remove the "Quick-edit" feature.
  • Replace the link in the toolbar with a "Edit on GitHub" link.

Screenshots

Before

image

After

image


How did you test this change?

  1. Open http://localhost:3000/en-US/docs/Learn/CSS/Building_blocks/Selectors
  2. Click on "Edit on GitHub"
  3. Open http://localhost:3000/fr/docs/Learn/CSS/Building_blocks/Selectors
  4. Click on "Edit on GitHub"

@caugner caugner requested a review from schalkneethling April 25, 2022 18:21
@caugner caugner added the chore A routine task. label Apr 25, 2022
@caugner caugner changed the title chore(app): remove quick-edit feature chore(document/toolbar): replace "Quick-Edit" by "Edit on GitHub" Apr 25, 2022
className="button"
title={`You're going to need to sign in to GitHub first (Opens in a new tab)`}

Choose a reason for hiding this comment

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

Nit

Suggested change
title={`You're going to need to sign in to GitHub first (Opens in a new tab)`}
title="Edit on GitHub. Opens in a new tab"

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 took it from here:
image

But maybe we don't need a title here at all? Since they're running yarn locally, they most likely have a GitHub account already, and opening in a new tab doesn't need to be advertised.

Choose a reason for hiding this comment

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

In general, it is a good idea for accessibility to state that a link opens in a new tab if one forces it with _blank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thank you, I have removed the _blank target, because there is no reason to enforce this.

Copy link

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

One suggestion but, other than that, this looks good to me. Thanks, @caugner 🎉

@caugner caugner merged commit 26f2b9f into mdn:main Apr 27, 2022
@caugner caugner deleted the remove-quick-edit branch April 27, 2022 16:43
OnkarRuikar pushed a commit to OnkarRuikar/yari that referenced this pull request Jun 2, 2022
…n#6065)

* chore(app): remove quick-edit feature
* chore(edit-actions): add "Edit on GitHub" link
@caugner caugner mentioned this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore A routine task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants