-
Notifications
You must be signed in to change notification settings - Fork 51
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
[DOC] README: Revise for new Consulting blog flow & general edits #704
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Would appreciate your review on these changes when you have a chance, @gabalafou -- thanks! |
a1ba746
to
ec65cf5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My inner librarian heart is warmed by these README updates, thank you!
I don't think any of my suggestions are blocking, but you might want to incorporate some of them, and we have to fix the merge conflict (banner vs overlay) before we can merge this.
`./apps/consulting/posts`, respectively. | ||
- **Static content** for each site lives under `./apps/labs/public` and | ||
`./apps/consulting/public`, respectively. | ||
- The `public` path element is removed when the site is built: for example, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! It's a common convention but it has caused confusion in the past.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen it ~50/50 with /static
being the magic folder name for static content. It took me a decent while with the codebase to realize it was doing the same thing.
README.md
Outdated
@@ -174,7 +176,7 @@ These are the concrete steps to follow to move your code from branch to branch: | |||
`develop` branch. | |||
- Consider doing a | |||
[squash-merge](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-pull-request-commits), | |||
especially if your pull request is relatively small, to keep a | |||
except if your pull request is relatively small, to keep a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I made my point here clearly.
If you have a one-line code change but several commits that are like:
sha1 fix code bug
sha2 typo
sha3 another typo
sha4 hopefully last typo
I would strongly prefer you to squash those commits.
But you have a PR that contains 100s of lines of code changes, which are chunked into commits, then maybe don't squash those commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point -- so, I'd think we'd want to squash all merges to develop
, then.
My reasoning for squashing aggressively is because we're working on GitHub, where the original chain of commits is retained in the closed PR, even when what gets merged to the base branch is a single squash.
It has the benefit of clean base branch history, while still retaining the full arc of the work process in the PR.
So, I'm inclined to strengthen this to something like "Please use GitHub's squash-merge functionality for all PRs to develop
."
For merges to main
, since release/hotfix branches can have weird history topology (e.g., a release gets hotfixed, then merges to main
, then merges to develop
) that would be hard to follow if it got squash-merged to either main
or develop
, I'm inclined to do simple commit merges there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll write out my case in a new commit here, see what you think on your next review pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to all of that
README.md
Outdated
should be a banner that explains that you are looking at a preview of the site. | ||
This helps reduce confusion when screenshots are shared. This banner should not | ||
be present on a production build of the site. | ||
should be a gray or yellow banner that indicates that you are looking at a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already changed the "banner" to "overlay"?
Do you branch off an older version of develop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR was opened well before that change in language. I'll need to rebase/merge from develop
& deal with any conflicts before this is ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
develop
merged in, conflicts fixed, and straggling instances of "banner" converted to "overlay" in this branch.
Should be taken care of on next commit.
README.md
Outdated
|
||
The preview/development environment banner should allow the user to toggle | ||
between seeing published versus draft/saved Storyblok content. The banner should | ||
change colors to indicate which of the two content preview modes the user is in | ||
(that is, whether they are seeing draft or published content from Storyblok). | ||
(that is, whether they are seeing draft (yellow banner) or published (gray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change open parens to dash so we don't have parentheses inside parentheses
(that is, whether they are seeing draft (yellow banner) or published (gray | |
—that is, whether they are seeing draft (yellow banner) or published (gray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed, will show in next commit.
README.md
Outdated
|
||
The preview/development environment banner should allow the user to toggle | ||
between seeing published versus draft/saved Storyblok content. The banner should | ||
change colors to indicate which of the two content preview modes the user is in | ||
(that is, whether they are seeing draft or published content from Storyblok). | ||
(that is, whether they are seeing draft (yellow banner) or published (gray | ||
banner) content from Storyblok). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
banner) content from Storyblok). | |
banner) content from Storyblok. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
README.md
Outdated
`<site>-blog/new-hello-world-post`. | ||
2. Add post images (feature, hero, and any for the post body) to | ||
`apps/<site>/public/posts/<post-name>`. | ||
3. Add a new `.md|.mdx` file inside the `app/<site>/posts` directory. Make sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3. Add a new `.md|.mdx` file inside the `app/<site>/posts` directory. Make sure | |
3. Add a new `.mdx` file inside the `app/<site>/posts` directory. Make sure |
As I mentioned in my other note, we should forget .md
. It's especially misleading because in normal Markdown you can create code blocks but "indented code does not work in MDX."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all of the posts for both blogs are .md
files. Should we rename them? Not great to have the churn in the file history.
If nothing else, any blogs not yet on develop
should be renamed to .mdx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it would be good to rename the .md
blog source files to .mdx
, a good time to do that might be as part of #746, since we're touching all those files to add the categories []
anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change locally, will reflect on next push
README.md
Outdated
format of the conventional commit. See the | ||
[Conventional Commits docs](https://www.conventionalcommits.org/en/v1.0.0/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format of the conventional commit. See the | |
[Conventional Commits docs](https://www.conventionalcommits.org/en/v1.0.0/). | |
[Conventional Commit format](https://www.conventionalcommits.org/en/v1.0.0/). |
I didn't actually know about this until today 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this change in both places it appears. Also capitalized 'Conventional Commit' in the flow of text, both places.
README.md
Outdated
`consulting 🤝` labels to the PR. | ||
6. Wait for someone on the website team to review the new blog post. If | ||
everything is ok, the PR will be merged to the `develop` branch. The blog | ||
post will then go live with the next site build (merge to `main`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
post will then go live with the next site build (merge to `main`). | |
post will then go live with the next production deployment (merge to `main`). |
Since "production deployment" is the language that Vercel uses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change, and several others for build -> deployment
.
There is an important semantic difference here, because e.g. if I run npm run build
locally, then it is building the site, but not deploying it. Please check to make sure that I've not messed these semantics up with my changes.
README.md
Outdated
format of the conventional commit. | ||
See the | ||
[Conventional Commits docs](https://www.conventionalcommits.org/en/v1.0.0/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
format of the conventional commit. | |
See the | |
[Conventional Commits docs](https://www.conventionalcommits.org/en/v1.0.0/). | |
[Conventional Commits format](https://www.conventionalcommits.org/en/v1.0.0/). |
README.md
Outdated
`consulting 🤝` labels to the PR. | ||
6. Wait for someone on the website team to review the new blog post. If | ||
everything is ok, the PR will be merged to the `develop` branch. The blog | ||
post will then go live with the next site build (merge to `main`). | ||
|
||
### Adding a new blog category | ||
|
||
1. Create a new feature branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that you didn't create this section but it feels a little odd to me that we have it at all. Because I'm guessing that a new blog category will be added with a blog post that wants a new category. And in that case it would make sense to me to push the new category and blog post together.
In fact, I would prefer NOT to have a developer follow this section to add a new blog category (without a corresponding blog post) because then the category will show up on the website without any blog posts under it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm solidly +1 to this, new categories only being created when posts are created that need them. I'll rewrite a draft for you to review.
Related, see #713.
Co-authored-by: gabalafou <[email protected]>
@gabalafou Ok, I've made edits per your requests. Ready for your next pass. Once you're satisfied with the content, I want to make a final text reflow pass over |
@gabalafou Annoying. The changes to the three blogs are due to automatic linting of staged files that husky does when I commit. I can't revert those files without figuring out how to temporarily disable that linting. <googles> Nice, |
Hm. With the migration to Wordpress complete, we should probably make note here of the |
👋 @gabalafou @pavithraes Closing to ~relinquish my ownership of the contribution, and because the changes are relatively stale since they originated before the Consulting site migration to WordPress. |
With the merge of #577, the Consulting blog is now operating on analogous GitHub-/Markdown-based machinery to the Labs blog.
The primary goal of this PR is to update the README to reflect this new Consulting blog workflow.
I also took the liberty of making some additional general edits to the README.
Closes #702.