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

Audit & fix Information.md #81

Closed
wants to merge 17 commits into from
Closed

Audit & fix Information.md #81

wants to merge 17 commits into from

Conversation

Yoda-Canada
Copy link
Contributor

Fixes #44
Steps done to modify and check Information.md:

  1. Fixed images work in dark modes. The memory model image's downward arrows and the letter in the bottom right corner are not clear in dark modes. I save the image in the image folder and link it to the information page. The issue is fixed.
  2. Added alt text for diagram images.
  3. The images can't display in all models. Change all the image path from "/img/.jpg" to "../../static/img/.jpg". The issue is fixed.
  4. Fix Frontmatter for the page to include id, title, slug, and description.
  5. Replace blockquote with Admonitions for clarity.
    image
    image

@AndreWillomitzer
Copy link
Contributor

AndreWillomitzer commented Nov 15, 2021

@Yoda-Canada A few things that may be causing the deployment to fail and I noticed:

  1. To fix the dark mode image issue, I would suggest wrapping them in a <span> tag and adding class "mdImg" to it. Then, when your change gets merged it will have the global CSS applied that I did in my merged PR:
.mdImg > p > img{
  background-color: #ffffff;
}
  1. I am not actually sure you needed to add <img> and <div> tags around the images. For my fix all I did was add the alt description to the Markdown image itself and then wrap it in a <span>. The difference between div and span are that divs are block elements, and span are inline. By using span, you won't mess with any mobile/screen resizing by accident.

  2. For regular text, I don't believe it is necessary to wrap them in <p> tags because we're using a Markdown file and so regular text that isn't preceeded by a symbol like ### is treated like normal text.

Otherwise, great job looks like you put tons of effort in to the changes 🚀 !

package.json Outdated
"execa": "^5.1.1",
"mr-pdf": "^1.0.7",
"wait-on": "^6.0.0"
=======
"docusaurus": "^1.14.7"
>>>>>>> issue-44
}

Choose a reason for hiding this comment

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

I think you forgot to erase the <<<<<<<<HEAD, ======, >>>>>>issue-44tag when you merge it to the main branch on your repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point that could be why deployment failed. Nice spot :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@suhhee1011 Thank you, I have modified it.

@@ -30,9 +34,12 @@ Since bits are too numerous to handle individually, modern computers transfer an

The fundamental addressable unit of RAM is the byte. One byte consists of 2 nibbles. Each nibble consists of 4 bits.

![](/img/bytenibbit.jpg)
<div align="left">
<img src="../../static/img/bytenibbit.jpg" alt="A bytes tree image" />
Copy link
Contributor

Choose a reason for hiding this comment

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

The alt text could be more descriptive. For instance: Tree showing relative sizes of bit, nibble and byte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, I have modified.

docs/A-Introduction/information.md Outdated Show resolved Hide resolved
@oliver-pham
Copy link
Contributor

I think you should remove hint-report/https-webhint-io.html and yarn.lock from your commit. These files are generated locally and not relevant to your changes.

sidebar_position: 2
title: Information
slug: /A-Introduction/information
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, great work on this Md file,
Something that I noticed was:

Slug should be standardized in small caps and without the "A-"
Check https://github.com/Seneca-ICTOER/IPC144/issues/64 for more details.

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, I have modified.

@Yoda-Canada
Copy link
Contributor Author

@Yoda-Canada A few things that may be causing the deployment to fail and I noticed:

  1. To fix the dark mode image issue, I would suggest wrapping them in a <span> tag and adding class "mdImg" to it. Then, when your change gets merged it will have the global CSS applied that I did in my merged PR:
.mdImg > p > img{
  background-color: #ffffff;
}
  1. I am not actually sure you needed to add <img> and <div> tags around the images. For my fix all I did was add the alt description to the Markdown image itself and then wrap it in a <span>. The difference between div and span are that divs are block elements, and span are inline. By using span, you won't mess with any mobile/screen resizing by accident.
  2. For regular text, I don't believe it is necessary to wrap them in <p> tags because we're using a Markdown file and so regular text that isn't preceeded by a symbol like ### is treated like normal text.

Otherwise, great job looks like you put tons of effort in to the changes 🚀 !

@AndreWillomitzer Thank you, I have modified all you mentioned.

@Yoda-Canada
Copy link
Contributor Author

I think you should remove hint-report/https-webhint-io.html and yarn.lock from your commit. These files are generated locally and not relevant to your changes.

@oliver-pham I have added these files in the .gitignore, thank you!

@Yoda-Canada
Copy link
Contributor Author

I think you should remove hint-report/https-webhint-io.html and yarn.lock from your commit. These files are generated locally and not relevant to your changes.

@oliver-pham yarn.lock shouldn't remove.

Copy link
Contributor Author

@Yoda-Canada Yoda-Canada left a comment

Choose a reason for hiding this comment

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

thx, I have modified.

@@ -30,9 +34,12 @@ Since bits are too numerous to handle individually, modern computers transfer an

The fundamental addressable unit of RAM is the byte. One byte consists of 2 nibbles. Each nibble consists of 4 bits.

![](/img/bytenibbit.jpg)
<div align="left">
<img src="../../static/img/bytenibbit.jpg" alt="A bytes tree image" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, I have modified.

package.json Outdated
"execa": "^5.1.1",
"mr-pdf": "^1.0.7",
"wait-on": "^6.0.0"
=======
"docusaurus": "^1.14.7"
>>>>>>> issue-44
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@suhhee1011 Thank you, I have modified it.


:::

### Sets of Bytes

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest escaping any * character that represents multiplication (line 122-128), as some of them will get converted into _ after being formatted with Prettier. See #56 for more info.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest escaping any * character that represents multiplication (line 122-128), as some of them will get converted into _ after being formatted with Prettier. See #56 for more info.

Did you mean the multiplication from line 115-122?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified them, thx

@Yoda-Canada
Copy link
Contributor Author

Since the commits can't be squashed to one, I close this PR and open a new PR #101. Thanks to all contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit & fix Information.md page
8 participants