-
Notifications
You must be signed in to change notification settings - Fork 41
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: fix guides according to vale linter recommendations #194
Conversation
Deployment has finished 👁️👄👁️ Your app is available here |
🚀 Your app has been updated and is available here |
🚀 Your app has been updated and is available here |
Former template was too detailed and not suited for quick changes like the changelog. Furthermore, already pre-filled templates seem to reduce author's verbosity. Since there isn't any way of enforcing the filling of a template on PR's like on issues on GitHub, if any additional information is needed it should be requested by the reviewer.
🚀 Your app has been updated and is available here |
🚀 Your app has been updated and is available here |
…n Laravel tutorial by {{% content/file %}}
🚀 Your app has been updated and is available here |
…nough for Laravel)
- Remove partials: partials are not specific enough for this guide and their style is too vague for specific frameworks. - Relocate partials: rewriting implied to relocate partials to get proper TOCs in pages, as sepcified in iss98. - Set guide in steps: use the steps shortcode to clarify the configuration and deployment process Discussion: Rewriting partials or deprecating their usage for the most part.
🚀 Your app has been updated and is available here |
🚀 Your app has been updated and is available here |
@cnivolle would you still merge my PR if I was a worm 🥹? |
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.
Nice job 👌🏻
🚀 Your app has been updated and is available here |
There is some problem with shortcodes that I do't understand yet. Looking into it : http://doc-review-pr-194.cleverapps.io/doc/applications/dotnet/ |
add space before title to try to fix the content rendering
🚀 Your app has been updated and is available here |
solved with a stupid edit (adding a space a top of the partial file) |
Your review app has been deleted 👋 |
Checklist
Pull request type
Please check the type of change your PR introduces:
Description
A summary of the changes
Laravel and Moddle guides have been rewritten for more clarity, partials used in them have been reindexed to get a proper TOC, and even removed from the page when they weren't useful or specific enough.
Changes
This PR contains a slight rework on the
/guides
section. These are the pages with editorial errors that will be fixed:Partials that were used (especially in
tutorial-laravael.md
have been reindexed has specified in this method by @cnivolle. However, I chose not to include them in the Laravel guide. The reasons are detailed in [commit 63d9c25] (63d9c25).Since reindexing partials could bring conflicts between this branch and main, I've rebased several times to avoid it, therefore this PR contains commits merged to main while working on this branch. Not sure this was smart for readability. When merging, commits should squashed with the content of the last commit.
Further work
This PR only targets rewriting the two aforementioned guides, not the partials. Work on all partials should be completed on a dedicated branch.
Why is this needed?
This is needed in order to improve readability of the guides, but also to progressively set up our own style to improve our editorial check up process in PR, as @davlgd evoked in this PR : #184 (comment)
Moodle guide was flagged by Vale, and I've followed the Laravel one last week: it was crowed with unstructured information and painful to navigate, in my opinion. New version should be clearer and easier to follow. Feel free to add suggestions or to point out errors.
Tests
Both Laravel and Moodle guides have been tested with demo apps:
Reviewes
Who should review these changes? @cnivolle @sebartyr