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

Fix small typos in the root md files #134609

Merged
merged 3 commits into from
Jun 23, 2022
Merged

Fix small typos in the root md files #134609

merged 3 commits into from
Jun 23, 2022

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Jun 16, 2022

Summary

This small PR fixes a few typos in 3 root level md files:

  • FAQ.md
  • STYLEGUIDE.mdx
  • TYPESCRIPT.md

Notes: I'm on week 3 at Elastic and this is my first PR. Please do not hesitate to close it if this PR is not desired, or if it isn't following the proper standards. I also could not find any guidance on how to format the commit message, so hopefully what I did is OK!

I'm not sure what version this PR should target, as it isn't really code related... should it sill have a label for the version (v8.4.0)?

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting documentation Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jun 20, 2022
@PhilippeOberti
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Thanks @PhilippeOberti, and welcome!

I'm not sure what version this PR should target, as it isn't really code related... should it sill have a label for the version (v8.4.0)?

For something like this, I think it's fine to just merge into main for the latest minor (currently targeting v8.4.0).

Added a few notes, lmk what you think.

STYLEGUIDE.mdx Outdated Show resolved Hide resolved
STYLEGUIDE.mdx Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
STYLEGUIDE.mdx Outdated Show resolved Hide resolved
@PhilippeOberti
Copy link
Contributor Author

Thanks @PhilippeOberti, and welcome!

I'm not sure what version this PR should target, as it isn't really code related... should it sill have a label for the version (v8.4.0)?

For something like this, I think it's fine to just merge into main for the latest minor (currently targeting v8.4.0).

Added a few notes, lmk what you think.

Thanks @lukeelmers! Changes made. While I reverted the . to : changes, I updated some other places which I believe could have used a :. Please let me know if that was unnecessary and I'll revert these as well 😄

One unrelated question: in my previous company we always wanted to have a single commit per PR to keep the commit log as clean as possible. It doesn't seem that Elastic is following this (at least in this Kibana repo). I just rebased against latest main and amended my commit, then force pushed. Hopefully that is ok...?

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Thanks for these updates -- I noticed a few other things and added comments below.

in my previous company we always wanted to have a single commit per PR to keep the commit log as clean as possible. It doesn't seem that Elastic is following this (at least in this Kibana repo). I just rebased against latest main and amended my commit, then force pushed. Hopefully that is ok...?

Kibana actually does follow this convention -- when a PR is approved, it is squashed into a single commit upon merging into main.

When you are still developing a PR, feel free to rebase & force push as much as you need to. But once others have started reviewing a PR, we recommend you avoid rebasing as it makes the history of changes much harder to follow for reviewers. Most folks find it easier to review a PR that is grouped into a series of logical commits than a single "Big Bang" commit with all the changes.

If you need to incorporate the latest upstream changes, you can just merge the latest main into your branch (we even have an automated way to do this: commenting @elasticmachine merge upstream on your PR will automatically update it for you).

STYLEGUIDE.mdx Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
TYPESCRIPT.md Outdated Show resolved Hide resolved
@PhilippeOberti
Copy link
Contributor Author

When you are still developing a PR, feel free to rebase & force push as much as you need to. But once others have started reviewing a PR, we recommend you avoid rebasing as it makes the history of changes much harder to follow for reviewers. Most folks find it easier to review a PR that is grouped into a series of logical commits than a single "Big Bang" commit with all the changes.

That makes sense. Just curious then, how do we decide what commit message takes priority? Maybe I'll just research how GitHub auto-squash functionality works :)

@lukeelmers sorry for all the extra formatting, I had enabled some auto-format on save with Webstorm and didn't catch them. Rookie mistake 😞 I've disabled it now...
I've undone all the unnecessary changes, we now have only the wanted changes. Sorry for wasting your time, I'll pay closer attention to this in the future!

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Changes LGTM, thanks for working through those!

how do we decide what commit message takes priority?

Github will default to the title of the PR, and include each commit message in the description, but the UI will let you change it to whatever you want :)

sorry for all the extra formatting, I had enabled some auto-format on save with Webstorm and didn't catch them

No worries! All you should need when working with the repo is eslint, and it should help keep everything formatted based on the linting & prettier rules we have defined for the repo. (Of course, it isn't going to format code inside markdown files like this, so 🤷)

@PhilippeOberti PhilippeOberti merged commit 8638f12 into main Jun 23, 2022
@PhilippeOberti PhilippeOberti deleted the fix-md-typos branch June 23, 2022 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting documentation release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants