gridcoin-community / Gridcoin-World-Site

Open source Gridcoin website for gridcoin.world
MIT License
4 stars 3 forks source link

Issue 13 fix guides page #19

Closed gridcat closed 2 years ago

gridcat commented 2 years ago

Fixes #13

Note: We need to improve the content of the guides. Since the solo crunching guide looks OK, the other two are definitely need some attention. While working on this issue I almost didn't touch the content itself as I can not consider myself as a tech writer (furthermore I ain't no native speaker) so I just removed non-related text from there. The guides on the original .us website for pool and staking-only aren't detailed enough and a little bit rather confusing, so perhaps some work needs to be done there as well to my opinion. Hence I suggest to not rely on the content of this PR but rather have additional issues to adjust content.

gridcat commented 2 years ago

Why are you putting underscores at the front of guides? The html and markdown have no _ at the start

The only reason is to keep thing consistent in the _includes folder.

gridcat commented 2 years ago

@barton2526 was it a question or suggestion to change? could you please be more specific? as I can see all files within the _includes folder are prefixed with underscore.

barton2526 commented 2 years ago

I'm not familiar enough with the original rationale to say. Perhaps @RoboticMind knows

RoboticMind commented 2 years ago

Underscores tell Jeykll to not try to build that file or folder directly.

Every file or directory beginning with the following characters: ., _ , # or ~ in the source directory will not be included in the destination folder

https://jekyllrb.com/docs/structure/

Technically the files in the include directory don't need to have and in front of them because the folder is already ignored. The files were like that before I started working with the gridcoin.us site and I've just kept it consistent with that. I would have no issue changing it to not have underscores in that folder as long as you change all the {%include _filename.extension %} to be {%include filename.extension %}, it would be fine.

gridcat commented 2 years ago

@RoboticMind I think it would be better to have separate issue for that as this is not related to this one. Plus it would be easier to test. wdyt?

gridcat commented 2 years ago

any other issues with this PR?

gridcat commented 2 years ago

@RoboticMind please have another look at this PR.

gridcat commented 2 years ago

And I think it is worth to deploy it despite the fact that some parts are missing. Those issues could be addressed as another PR. I believe this improvements worth to have deployed, it should be better anyway.

gridcat commented 2 years ago

I guess the proper way would be to Squash during the merge. I believe github should offer this choice. My attempt seems failed.

barton2526 commented 2 years ago

That option would result in one commit. I think Robotic meant to reduce 22 to a more manageable number, but still keep them modular (ie >1)

gridcat commented 2 years ago

I understand, so I tried before your message doing this in a regular way (rebase ~10, the mark as squash) but seems like because this branch is already on a remote I can not rewrite the history. I may doing something wrong so I can use some help here.

barton2526 commented 2 years ago

You need to squash locally and then force push to gridcat:issue-13-fix-guides-page

gridcat commented 2 years ago

@barton2526 thank you, --force did a trick!

gridcat commented 2 years ago

@RoboticMind can we have it merged now?

RoboticMind commented 2 years ago

Merged