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

Fixes Parts of #3783 #3784

Closed
wants to merge 3 commits into from
Closed

Fixes Parts of #3783 #3784

wants to merge 3 commits into from

Conversation

CDNRae
Copy link

@CDNRae CDNRae commented Feb 28, 2019

Description of the Change

Fixes some issues outlined in #3783.

  • Modified the alt text for the mocha logo and gave it an ID attribute, which is then used in styles.css to set the logo's width and height.
  • Added the external attribute to the link to the Creative Commons License
  • Removed the shortcut attribute from the favicon.

Alternate Designs

N/A

Why should this be in core?

N/A

Benefits

It improves the website.
Removes meaningless attributes while updating HTML/CSS to more modern standards.

Possible Drawbacks

None to knowledge.

Applicable issues

Partial #3783.
semver-patch

@coveralls
Copy link

coveralls commented Feb 28, 2019

Coverage Status

Coverage increased (+0.03%) to 91.713% when pulling 92ba015 on rscotchmer:issue-3783 into 22831c5 on mochajs:master.

docs/css/style.css Outdated Show resolved Hide resolved
@plroebuck
Copy link
Contributor

Needs tweek to stylesheet to match what was there... change footer span to footer div and see if that makes your changes match the deployed site.

footer div {

@plroebuck plroebuck added area: documentation anything involving docs or mochajs.org nice to have enhancement proposal of low priority semver-patch implementation requires increase of "patch" version number; "bug fixes" area: website involving mochajs.org, but not necessarily involving docs labels Mar 1, 2019
@@ -23,10 +23,9 @@
<h1>
<a href="/">
<img
id="mocha-logo"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't use an ID for targeting this. Just a logo class or something.

src="/images/mocha-logo.svg"
alt="Mocha"
width="192"
height="192"
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the dimensions; the HTML is rendered first so it's always better to have the dimensions here.

@@ -39,6 +39,11 @@ header {
padding-top: 20px;
}

#mocha-logo {
width:192px;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is moot if you just keep the dimensions in the HTML, which IMO is better.

@@ -13,7 +13,7 @@
<link rel="stylesheet" href="css/normalize.css" />
<link rel="stylesheet" href="css/style.css" />
<link rel="stylesheet" href="css/prism.css" />
<link rel="shortcut icon" href="favicon.ico" />
<link rel="icon" href="favicon.ico" />
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW you can remove the /> from the HTML tags here. It's not needed in HTML5+.

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to mention, shortcut icon is needed for old IE. Which AFAICT since html5shiv is loaded, it is still supported.

If not, that's OK, but both should be dropped.

@XhmikosR XhmikosR mentioned this pull request Mar 3, 2019
@Munter Munter closed this in #3807 Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: documentation anything involving docs or mochajs.org area: website involving mochajs.org, but not necessarily involving docs nice to have enhancement proposal of low priority semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants