Skip to content
This repository was archived by the owner on Apr 22, 2021. It is now read-only.

Conversation

@viraji
Copy link
Contributor

@viraji viraji commented Aug 6, 2020

Changed nav bar to:
Home | Licences | Community | Download | Press
from:
Home | About | Licences | Contribute | Contact Us
as per Issue #14 described in:
https://docs.google.com/document/d/1KVU44BA37tu-4DXYM99diCeqI6FeDiOFYQp5zLwl8vU/edit?usp=sharing

@viraji viraji marked this pull request as ready for review August 7, 2020 15:28
@viraji
Copy link
Contributor Author

viraji commented Aug 7, 2020

All changes in #14 done as per the agreed upon doc at:
https://docs.google.com/document/d/1KVU44BA37tu-4DXYM99diCeqI6FeDiOFYQp5zLwl8vU/edit?usp=sharing

contact.md Outdated
* #random: Non-work-related stuff

### Real-time conference calls
### Real-Time Conference Calls
Copy link
Member

Choose a reason for hiding this comment

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

Is title case written down as our standard somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is title case written down as our standard somewhere?
Not sure, but all other headings were in title case except for these two that I changed - so I changed these as well to make the page consistent.

Copy link
Member

Choose a reason for hiding this comment

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

We follow Google's style guide, which specifies sentence case for titles and headings: https://developers.google.com/style/capitalization#capitalization-in-titles-and-headings

Copy link
Member

Choose a reason for hiding this comment

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

They'll all need to be updated to sentence case, then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed accordingly.

Comment on lines +13 to +22
For most decisions, a motion is raised within one of TheGoodDocsProject open forums (slack, email list, etc) and those present vote and decide. Should a decision:

Be strategic or impact many within TheGoodDocsProject,
Or if there is contention in deciding, and after discussion at least one member is still strongly against the motion, by continuing to vote -1,

Then the motion should be referred to the PSC.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For most decisions, a motion is raised within one of TheGoodDocsProject open forums (slack, email list, etc) and those present vote and decide. Should a decision:
Be strategic or impact many within TheGoodDocsProject,
Or if there is contention in deciding, and after discussion at least one member is still strongly against the motion, by continuing to vote -1,
Then the motion should be referred to the PSC.
For most decisions, a motion is raised within one of TheGoodDocsProject open forums (slack, email list, etc) and those present vote and decide. A decision should be referred to the PSC only if:
- The decision is strategic or could impact many people within TheGoodDocsProject
- If there is contention in deciding, and after discussion at least one member is still strongly against the motion, by continuing to vote -1

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to worry about fixing this lift-and-shift of the decisions page in this iteration.
It is about to get updated again based on https://docs.google.com/document/d/1hwdA2Gh3ePUFXk4juu7mSoAraxV-BDiarXaPCOY6K3w/edit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

@Loquacity Loquacity requested a review from camerons August 9, 2020 00:30
README.md Outdated
@@ -0,0 +1,2 @@
# thegooddocsproject.github.io
Forking The Good Docs Project website
Copy link
Member

Choose a reason for hiding this comment

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

As you are doing a lift-and-shift, and this file doesn't contain any value. I'd suggest that we don't include this README.md file. (We can add it later.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted and readme.md deleted.

Copy link
Member

@camerons camerons left a comment

Choose a reason for hiding this comment

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

I've added some comments.
I'm embarrassingly unskilled at setting up a local web server and hope to work that out soon. (Help welcomed).
I'm hoping someone can view these changes rendered in a web page and confirm they look okay and links work.

<a href="https://thegooddocsproject.dev/licences.html" style="color:#ffffff;">Licences</a> |
<a href="https://thegooddocsproject.dev/contribute.html" style="color:#ffffff;">Contribute</a> |
<a href="https://thegooddocsproject.dev/contact.html" style="color:#ffffff;">Contact Us</a> |
<a href="https://thegooddocsproject.dev/licences.html" style="color:#ffffff;">Licences</a>
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this hyperlinks relative (licences.html) instead of absolute (https://thegooddocsproject.dev/licences.html)?
Doing so will mean we can test the site in a staging environment, or local environment.
Apply the same comment to other links too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Relative links weren't applied, initially wouldn't work for preview/staging. I made the adjustments.

community.md Outdated



* [Contribute](https://thegooddocsproject.groups.io/g/contribute.md)
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked these links in a rendered website? I haven't but suspect that contribute.html will find the correct page, but contribute.md will get a 404.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was unable to test in a rendered website, and so followed existing code. The link has now been changed accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested these links, majority of them go to the groups.io as a .md link and don't load. I see there were new .md files added that match. So I constructed local links.

contact.md Outdated
We are nice to each other, in line with our [Code of Conduct](https://github.com/thegooddocsproject/governance/blob/master/CodeOfConduct.md).

### Who are we?
### Who are We?
Copy link
Member

Choose a reason for hiding this comment

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

We should use sentence-case as per https://developers.google.com/style/capitalization

Copy link
Contributor Author

@viraji viraji left a comment

Choose a reason for hiding this comment

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

Review changes have now been attended to, please review the PR now.

Copy link
Contributor

@mgan59 mgan59 left a comment

Choose a reason for hiding this comment

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

Ran through these changes. Sidenote, all pages were missing front-matter which is required. As a result the build only generate markdown in the _site folder as indicated in this stackoverflow. I have no idea how this site builds/deploys without the front-matter. I went through and added

---
layout: default
---

To all pages which means they now use the _layout/default.html

<a href="https://thegooddocsproject.dev/licences.html" style="color:#ffffff;">Licences</a> |
<a href="https://thegooddocsproject.dev/contribute.html" style="color:#ffffff;">Contribute</a> |
<a href="https://thegooddocsproject.dev/contact.html" style="color:#ffffff;">Contact Us</a> |
<a href="https://thegooddocsproject.dev/licences.html" style="color:#ffffff;">Licences</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Relative links weren't applied, initially wouldn't work for preview/staging. I made the adjustments.

community.md Outdated



* [Contribute](https://thegooddocsproject.groups.io/g/contribute.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tested these links, majority of them go to the groups.io as a .md link and don't load. I see there were new .md files added that match. So I constructed local links.

viraji and others added 14 commits August 23, 2020 07:17
Adding new file for Community
Changed all headings to title case for consistency.
Content to be filled in later.
Calling existing links.
Changed file extension from .md to .html.
Deleting the file following Cameron's comment: " As you are doing a lift-and-shift, and this file doesn't contain any value. I'd suggest that we don't include this README.md file. (We can add it later.)"
@mgan59
Copy link
Contributor

mgan59 commented Aug 23, 2020

There were conflicts with index.md from master. I did a REBASE with a --force push. I'm assuming that these changes are final. But if you need/want to continue working on this branch you will need to rebuild your local branch.

@mgan59
Copy link
Contributor

mgan59 commented Aug 23, 2020

github has a limit 10 megs, so broke up the review gifs.

Verification Top NavBar has relative links that work

good-docs-review-pr-35-part1

Verification of Community Page -- bunch of additional relative links to new pages (Roles, Decision Logs)

good-docs-pr-35-community

public_suffix (>= 2.0.2, < 4.0)
colorator (1.1.0)
concurrent-ruby (1.1.5)
em-websocket (0.5.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how these changes crept in, I had stashed them... if we have a issue with the build I can undo them.

@camerons camerons merged commit 5994f61 into thegooddocsproject:master Aug 23, 2020
@camerons
Copy link
Member

Provided my approval based on screenshots from Morgan, review of code changes, and my trust of Morgan's review process.
(I don't have a local version to check.)

@mgan59 mgan59 mentioned this pull request Aug 23, 2020
3 tasks
@Loquacity
Copy link
Member

🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants